date:20140425

Philippe Vaucher wrote:
  I think you are on the right track but the solution is not to shrug 
  shoulders.
  We should acknowledge there are serious problems with the interface, list 
  them,
  and try to fix them. One example is `git add $tracked_file` being wrong, it
  should be `git stage $tracked_file`.
 
 I agree. The stage area is a very important concept in git, why not
 talk git commands that refers to it? Then we could add flags like
 --new-files or --deleted-files for better granularity than the current
 --all flag.

Like this:
http://thread.gmane.org/gmane.comp.version-control.git/236127

  The real problem is that the core developers of Git don't acknowledge these
  user-interface issues, according the them the interface doesn't require any
  major changes. Which goes contrary to what most of the world believes.
 
 I think most people agree with these interfaces issues but it's a hard
 problem to solve, so  they are reluctant to talk about it because they
 fear the can of worm. If someone came with a good solution most people
 would be willing to consider it.
 
 I think starting by documenting the issues is a good idea, maybe on a
 wiki, and start some draft of a proposed solution that would improve
 in an iterative process.

Yes, it would be good to document these issues, but it wouldn't matter if the
developers ignore them.

For example the move away from the 'index' name was backed up by literally
everyone, except Junio, so it doesn't matter if the issue is documented, and
there are patches with good solutions, if Junio thinks it's not an issue; it's
ignored.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

2014-04-25 Thread Theodore Ts'o

On Fri, Apr 25, 2014 at 09:48:53AM +0200, Philippe Vaucher wrote:
 
 I agree. The stage area is a very important concept in git, why not
 talk git commands that refers to it? Then we could add flags like
 --new-files or --deleted-files for better granularity than the current
 --all flag.

One caution: The term stage/staged is already a little overloaded.
We generally use the word staged to refer to changes that are in the
index, but the term stage as a noun generally refers to referencing
the different versions of a file during a merge operation (cf git
ls-files --stage).

 I think starting by documenting the issues is a good idea, maybe on a
 wiki, and start some draft of a proposed solution that would improve
 in an iterative process.

And it would be nice if the issues were discussed in a way that
acknowledged that all changes have tradeoffs, both positive and
negative, and to clearly articulate whether the concern is just
someone going uh, 'index' is a wierd term, but once they learn it,
it's pretty clear, versus a case where there is continuous confusion
due to overloaded meanings, or for people for whom English might not
be the first language.

And most importantly, to avoid rheteroic.  In fact, given that strong
use of rhetoric is often used to disguise a weakness of a position
that can't be defended using logic and data, someone who tries to win
arguments using the last post wins style of discourse, and a heavy
use of rhetoric, may find that people just simply decide that it's a
better use of their time not to engage and to just kill the entire
thread.

Regards,

- Ted
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

2014-04-25 Thread Theodore Ts'o

On Fri, Apr 25, 2014 at 04:23:43PM +0200, Philippe Vaucher wrote:
 
 I agree, but I think it's better than index tho. That one is heavily
 overloaded and easily confused with other meaning in other softwares.

There is a big difference between being used in a difference sense
than other software --- there is a one-time learning curve after which
point people can generally understand that a term in a given context
has a single meaning --- and when we have two very easily confused
terms (i.e., stage versus staged) or a single identical term,
overloaded within a single context.  So I'm much more worried about
the git documentation using the same term or two closely related terms
in an overloaded fashion, much more than I am with index meaning one
thing for databases, and another thing for book publishers, and yet
another for compilers.

 Yes, of course there should be a list of both positive and negative
 tradeoffs. But I think the overloaded argument can be easily solved
 by renaming one of the overloads.

And renaming one of a term also has costs, especially if it is one
that is in use in large amounts of documentation, both in the git man
pages, and in web pages across the web.

And my plea for data extends even here.  For example, things like
this:

www.google.com/trends/explore#q=git%20staging%20area%2C%20git%20indexcmpt=q

 Unfortunately yes, I see many people being silly in order to win
 arguments, both in the pro-changes and against-changes side of the
 discussion. I'd be much simpler to simply gather arguments on some
 wiki and eventually do a vote when the list is complete about the
 proposed change.

Voting is not a good way to do software development.  That way lies
people wanting to whip up clueless folks using rhetoric (exhibit one:
Fox News) to vote and so it's not necessarily the best way to make
thoughtful decisions.  Using hard data, including possibly formal UX
experiments, is a much better way to make such decisions.

Cheers,

- Ted

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Apr 2014, #07; Tue, 22)

2014-04-25 Thread brian m. carlson

On Tue, Apr 22, 2014 at 04:12:42PM -0700, Junio C Hamano wrote:
 * bc/blame-crlf-test (2014-02-18) 1 commit
  - blame: add a failing test for a CRLF issue.
 
  I have a feeling that a fix for this should be fairly isolated and
  trivial (it should be just the matter of paying attention to the
  crlf settings when synthesizing the fake commit)---perhaps somebody
  can squash in a fix to this?

Last time I looked at this, I couldn't come up with an obvious fix for
the problem, or I would have included one.  I'll try to have another go
at it this weekend.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: Digital signature

Re: Harmful LESS flags

Hi,

Matthieu Moy wrote:

 I am personally in favor of changing the default to drop the S. Silently
 hiding stuff from the user's eyes is really bad. With good coding
 standard and reasonable terminal size, it actually doesn't matter.

Just for clarity: no, when we are talking about well formatted code,
-S is actually a way better interface.

That's because indentation matters and makes it easy to take in code
structure at a glance, long lines that get cut off by the margin stick
out like a sore thumb already, and lines wrapped at an arbitrary
character are even more distracting to the point of being useless.

In practice I believe the Silently hiding stuff concern is much
harder to solve.  In the case of malicious code that opened this
thread, I think a marker on the right margin would reveal the
whitespace more clearly than wrapping that the reader may or may not
notice.

Luckily, it is very easy to switch between the two views on the fly
--- in an already-open less window, you just type '-' + 'S'.  In the
spirit of not overriding tool defaults when there is not a strong
reason to do so, I agree that if someone writes a patch to drop
the 'S' I would probably like it.

[...]
 I do not think we particularly need a transition plan here: it's purely
 a user-interface thing, not something that may break any script or other
 tool.

Agreed  a note in release notes and making sure the documentation
reflects the new default should be enough.

Thanks and hope that helps,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Harmful LESS flags

2014-04-25 Thread David Kastrup

Jonathan Nieder jrnie...@gmail.com writes:

 Matthieu Moy wrote:

 I am personally in favor of changing the default to drop the S. Silently
 hiding stuff from the user's eyes is really bad. With good coding
 standard and reasonable terminal size, it actually doesn't matter.

 Just for clarity: no, when we are talking about well formatted code,
 -S is actually a way better interface.

When we are talking about well-formatted code, -S does not matter either
which way.

 That's because indentation matters and makes it easy to take in code
 structure at a glance, long lines that get cut off by the margin stick
 out like a sore thumb already, and lines wrapped at an arbitrary
 character are even more distracting to the point of being useless.

Lines which are cut off are not to the point of being useless, they
_are_ useless.

I am not arguing that wrapped lines are pretty.  And I also consider the
malicious or hiding angle at best a marginal concern.

Overriding less' defaults should only be done for unequivocal benefits,
and in this case I consider the result actually more of a detriment than
anything else.

-- 
David Kastrup
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

Hi,

David Kastrup wrote:
 Javier Domingo Cansino javier...@gmail.com writes:

 = Reject non-fast-forward pulls by default =
 Not having this introduced yet allows newbie people to use git with
 just 4 commands, without bothering around with fetch and merge and so.

 If you have a gun lying around your house, you should turn the safety
 catch off or the children will not be able to use it without
 instruction.

I probably missed a subtlety, but the above comment reminded me of
some netiquette I think this list is starting to forget.  If I have
misread it, please let me know and skip the rest of this message.

This is a comment about style, not substance.  Like coding style,
discussion style matters as a way of making life easier for
maintainers and new contributors, and different projects have subtly
different practices.

On the git list, the rule is simple.  Feel free to grumble, but make
sure there is some contribution in the same message.  For example,
after the confusing gun analogy you can say How about this patch?
and people reading can follow up by reviewing that patch and
potentially getting it applied.

But what if there's already a patch doing what I want to do? you
might ask.  No problem.  Link to the patch and say I think this patch
should be revisited, or even better, re-send it with some notes about
how previous review comments have been addressed or remain to be
addressed.

How do I get feedback on design of a new change without wasting a lot
of time coding something that might be a bad idea?  Glad you asked.
Sometimes instead of a patch, a better contribution is a detailed
explanation of a design to be reviewed and adopted.  In this case, do
try to be clear about whether you'll have enough time to implement the
result or will be relying on others so people know how much time to
devote to working out the details.

What about feature requests?  I might have an idea for improving git
that no one's had before but I don't have a detailed design in mind.
Sure, that can be a good contribution.  Just treat it as a gift ---
don't assume anyone is going to implement it for you if they're not
interested.

What about reminders about known bugs?  There's this regression and
it would not be right to release without fixing it but I think it's
fallen through the cracks.  Yes, good contribution.

What about jokes?  Sure, making people laugh is productive.

What about cryptic grumbling?  Sorry, unless you are grumbling to
get input on how to improve git's documentation or code, it's not
enough to be worth sending an email to this list.

Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Harmful LESS flags

David Kastrup wrote:
 Jonathan Nieder jrnie...@gmail.com writes:

 Just for clarity: no, when we are talking about well formatted code,
 -S is actually a way better interface.

 When we are talking about well-formatted code, -S does not matter either
 which way.

Sorry for the lack of clarity.  I believe well-formatted code can
contain long lines.  For example, sometimes a message + the printf to
print it and indentation move past the right margin.

If I wasn't talking about long lines, why would I have replied in the
first place?

[...]
 Overriding less' defaults should only be done for unequivocal benefits,

We agree here.  So, does someone who actually wants this change want to
propose a patch? :)

Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

2014-04-25 Thread David Kastrup

Jonathan Nieder jrnie...@gmail.com writes:

 Hi,

 David Kastrup wrote:
 Javier Domingo Cansino javier...@gmail.com writes:

 = Reject non-fast-forward pulls by default =
 Not having this introduced yet allows newbie people to use git with
 just 4 commands, without bothering around with fetch and merge and so.

 If you have a gun lying around your house, you should turn the safety
 catch off or the children will not be able to use it without
 instruction.

 I probably missed a subtlety, but the above comment reminded me of
 some netiquette I think this list is starting to forget.  If I have
 misread it, please let me know and skip the rest of this message.

 This is a comment about style, not substance.  Like coding style,
 discussion style matters as a way of making life easier for
 maintainers and new contributors, and different projects have subtly
 different practices.

 On the git list, the rule is simple.  Feel free to grumble, but make
 sure there is some contribution in the same message.  For example,
 after the confusing gun analogy you can say How about this patch?
 and people reading can follow up by reviewing that patch and
 potentially getting it applied.

Uh, Javier was commenting on a number of concrete proposals regarding
the subject line What is missing from Git v2.0 and quoted Felipe
directly.  As you seem to have lost the context, let me requote the
respective portion:

From: Felipe Contreras felipe.contre...@gmail.com
Subject: What is missing from Git v2.0
Newsgroups: gmane.comp.version-control.git
To: git@vger.kernel.org
Date: Sun, 20 Apr 2014 17:41:05 -0500 (4 days, 17 hours, 12 minutes ago)

[...]

= Reject non-fast-forward pulls by default =

Many new-comers end up making merges by mistake when they pull
because they don't understand what is a non-fast-forward and what
they should actually be doing. Most people, even Linus Torvalds,
agreed that by default `git pull` should fail and guide the user,
instead of silently making a merge which might not be what the user
wants (even though he doesn't know it), and messing up the project's
history, which affects other people.

The patches were sent, the issues were addressed, people agreed, and
yet nothing happened.

[3][4]

[...]

[3] http://thread.gmane.org/gmane.comp.version-control.git/240030
[4] http://thread.gmane.org/gmane.comp.version-control.git/235981


 How do I get feedback on design of a new change without wasting a lot
 of time coding something that might be a bad idea?  Glad you asked.

I didn't.

 What about reminders about known bugs?  There's this regression and
 it would not be right to release without fixing it but I think it's
 fallen through the cracks.  Yes, good contribution.

 What about jokes?  Sure, making people laugh is productive.

 What about cryptic grumbling?  Sorry, unless you are grumbling to
 get input on how to improve git's documentation or code, it's not
 enough to be worth sending an email to this list.

 Hope that helps,

No need to get condescending if you did not get the joke.

-- 
David Kastrup
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

2014-04-25 Thread Philippe Vaucher

Yes, of course there should be a list of both positive and negative
tradeoffs. But I think the overloaded argument can be easily solved
by renaming one of the overloads.

And renaming one of a term also has costs, especially if it is one
that is in use in large amounts of documentation, both in the git man
pages, and in web pages across the web.

It's just impossible to change terms and have previous documentation
still be valid. Sure, you can list it in the cons section as an
argument, but it's not very convincing in itself because it applies to
pretty much any interface changes. I think the main idea is to
_improve_ the interface, which means rename things so it is more
consistent and so concepts are easier for new comers to grasp. We
could still support old terms for a while and eventually deprecate
them.

www.google.com/trends/explore#q=git%20staging%20area%2C%20git%20indexcmpt=q

I think this comparison is a bit bogus, searching for git stage
yields more accurate results, we can see the searches are related:

http://www.google.com/trends/explore#q=git%20staging%20area%2C%20git%20index%2C%20git%20stagecmpt=q

Unfortunately yes, I see many people being silly in order to win
arguments, both in the pro-changes and against-changes side of the
discussion. I'd be much simpler to simply gather arguments on some
wiki and eventually do a vote when the list is complete about the
proposed change.

Voting is not a good way to do software development. That way lies
people wanting to whip up clueless folks using rhetoric (exhibit one:
Fox News) to vote and so it's not necessarily the best way to make
thoughtful decisions. Using hard data, including possibly formal UX
experiments, is a much better way to make such decisions.

Interesting, but then who's to say which data is more important than
anothers? For example, I consider improving the interface to be more
important than having old documentation on blogs/tutorial for a while
until people catch up, but maybe you consider old documentation to be
more important... who decides what really counts? I guess it's a mix
of general consensus and old timers credibility.

Anyway, having some data as a list of pros/cons would greatly simplify
the debate.

Philippe
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[PATCH v3 11/19] tag.c: use ref transactions when doing updates

Change tag.c to use ref transactions for all ref updates.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/tag.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 40356e3..dd53fb4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -488,7 +488,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf ref = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
-   struct ref_lock *lock;
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
@@ -496,6 +495,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+   struct ref_transaction *transaction;
+   char *err = NULL;
struct option options[] = {
OPT_CMDMODE('l', list, cmdmode, N_(list tag names), 'l'),
{ OPTION_INTEGER, 'n', NULL, lines, N_(n),
@@ -641,11 +642,12 @@ int cmd_tag(int argc, const char **argv, const char 
*prefix)
if (annotate)
create_tag(object, tag, buf, opt, prev, object);
 
-   lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
-   if (!lock)
-   die(_(%s: cannot lock the ref), ref.buf);
-   if (write_ref_sha1(lock, object, NULL)  0)
-   die(_(%s: cannot update the ref), ref.buf);
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_update(transaction, ref.buf, object, prev,
+  0, !is_null_sha1(prev)) ||
+   ref_transaction_commit(transaction, NULL, err))
+ die(_(%s: cannot update the ref: %s), ref.buf, err);
if (force  !is_null_sha1(prev)  hashcmp(prev, object))
printf(_(Updated tag '%s' (was %s)\n), tag, 
find_unique_abbrev(prev, DEFAULT_ABBREV));
 
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 07/19] refs.c: remove the onerr argument to ref_transaction_commit

Since we now pass meaningful error messages back from ref_transaction_commit
on failures, we no longer need to provide a onerr argument. The callers can now
on commit failures die() with a meaningful, and in most cases even better than
before, error message.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c |  3 +--
 refs.c   | 22 +++---
 refs.h   |  3 +--
 3 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 47c9b53..df3cd92 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -368,8 +368,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
-   if (ref_transaction_commit(transaction, msg, err,
-  UPDATE_REFS_QUIET_ON_ERR))
+   if (ref_transaction_commit(transaction, msg, err))
die(update_ref failed: %s, err);
return 0;
}
diff --git a/refs.c b/refs.c
index 7562151..0e2df81 100644
--- a/refs.c
+++ b/refs.c
@@ -3397,8 +3397,7 @@ static int ref_update_compare(const void *r1, const void 
*r2)
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
-   char **err,
-   enum action_on_err onerr)
+   char **err)
 {
int i;
for (i = 1; i  n; i++)
@@ -3410,22 +3409,13 @@ static int ref_update_reject_duplicates(struct 
ref_update **updates, int n,
snprintf(*err, PATH_MAX + 41, str,
 updates[i]-refname);
}
-   switch (onerr) {
-   case UPDATE_REFS_MSG_ON_ERR:
-   error(str, updates[i]-refname); break;
-   case UPDATE_REFS_DIE_ON_ERR:
-   die(str, updates[i]-refname); break;
-   case UPDATE_REFS_QUIET_ON_ERR:
-   break;
-   }
return 1;
}
return 0;
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
-  const char *msg, char **err,
-  enum action_on_err onerr)
+  const char *msg, char **err)
 {
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3443,7 +3433,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
-   ret = ref_update_reject_duplicates(updates, n, err, onerr);
+   ret = ref_update_reject_duplicates(updates, n, err);
if (ret)
goto cleanup;
 
@@ -3455,7 +3445,8 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
   (update-have_old ?
update-old_sha1 : NULL),
   update-flags,
-  update-type, onerr);
+  update-type,
+  UPDATE_REFS_QUIET_ON_ERR);
if (!update-lock) {
if (err) {
const char *str = Cannot lock the ref '%s'.;
@@ -3476,7 +3467,8 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
ret = update_ref_write(msg,
   update-refname,
   update-new_sha1,
-  update-lock, err, onerr);
+  update-lock, err,
+  UPDATE_REFS_QUIET_ON_ERR);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
diff --git a/refs.h b/refs.h
index 615c73d..f3db632 100644
--- a/refs.h
+++ b/refs.h
@@ -272,8 +272,7 @@ void ref_transaction_delete(struct ref_transaction 
*transaction,
  * why a commit failed. This string must be free()ed by the caller.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-  const char *msg, char **err,
-  enum action_on_err onerr);
+  const char *msg, char **err);
 
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to

[PATCH v3 02/19] refs.c: allow passing NULL to ref_transaction_free

Allow ref_transaction_free to be called with NULL and in extension allow
ref_transaction_rollback to be called for a NULL transaction.

This allows us to write code that will
  if ( (!transaction ||
ref_transaction_update(...))  ||
  (ref_transaction_commit(...)  !(transaction = NULL)) {
  ref_transaction_rollback(transaction);
  ...
  }

In this case transaction is reset to NULL IFF ref_transaction_commit() was
invoked and thus the rollback becomes ref_transaction_rollback(NULL) which
is safe. IF the conditional triggered prior to ref_transaction_commit()
then transaction is untouched and then ref_transaction_rollback(transaction)
will rollback the failed transaction.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/refs.c b/refs.c
index 138ab70..2d83ef6 100644
--- a/refs.c
+++ b/refs.c
@@ -3303,6 +3303,9 @@ static void ref_transaction_free(struct ref_transaction 
*transaction)
 {
int i;
 
+   if (!transaction)
+   return;
+
for (i = 0; i  transaction-nr; i++)
free(transaction-updates[i]);
 
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 12/19] replace.c: use the ref transaction functions for updates

Update replace.c to use ref transactions for updates.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/replace.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index b62420a..cf0f56d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -129,7 +129,8 @@ static int replace_object(const char *object_ref, const 
char *replace_ref,
unsigned char object[20], prev[20], repl[20];
enum object_type obj_type, repl_type;
char ref[PATH_MAX];
-   struct ref_lock *lock;
+   struct ref_transaction *transaction;
+   char *err = NULL;
 
if (get_sha1(object_ref, object))
die(Failed to resolve '%s' as a valid ref., object_ref);
@@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const 
char *replace_ref,
else if (!force)
die(replace ref '%s' already exists, ref);
 
-   lock = lock_any_ref_for_update(ref, prev, 0, NULL);
-   if (!lock)
-   die(%s: cannot lock the ref, ref);
-   if (write_ref_sha1(lock, repl, NULL)  0)
-   die(%s: cannot update the ref, ref);
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_update(transaction, ref, repl, prev,
+  0, !is_null_sha1(prev)) ||
+   ref_transaction_commit(transaction, NULL, err))
+ die(_(%s: failed to replace ref: %s), ref, err);
 
return 0;
 }
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer

Change ref_transaction_commit to take a pointer to a pointer for the
transaction. This allows us to clear the transaction pointer from within
ref_transaction_commit so that it becomes NULL in the caller.

This makes transaction handling in the callers much nicer since instead of
having to write horrible code like :
t = ref_transaction_begin();
if ((!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
   !!oldval)) ||
(ref_transaction_commit(t, action, err)  !(t = NULL))) {
ref_transaction_rollback(t);

we can now just do the much nicer
t = ref_transaction_begin();
if (!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
   !!oldval) ||
ref_transaction_commit(t, action, err)) {
ref_transaction_rollback(t);

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 branch.c |  4 ++--
 builtin/commit.c |  2 +-
 builtin/replace.c|  2 +-
 builtin/tag.c|  2 +-
 builtin/update-ref.c |  2 +-
 fast-import.c|  7 +++
 refs.c   | 18 ++
 refs.h   |  3 ++-
 sequencer.c  |  7 +++
 9 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/branch.c b/branch.c
index 23cde1e..5d68467 100644
--- a/branch.c
+++ b/branch.c
@@ -303,14 +303,14 @@ void create_branch(const char *head,
if (!transaction ||
ref_transaction_update(transaction, ref.buf, sha1,
   NULL, 0, 0) ||
-   ref_transaction_commit(transaction, msg, err))
+   ref_transaction_commit(transaction, msg, err))
  die_errno(_(%s: failed to write ref: %s),
ref.buf, err);
} else {
if (!transaction ||
ref_transaction_create(transaction, ref.buf, sha1,
   0) ||
-   ref_transaction_commit(transaction, msg, err))
+   ref_transaction_commit(transaction, msg, err))
  die_errno(_(%s: failed to write ref: %s),
ref.buf, err);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 7e4c306..3142827 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1682,7 +1682,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
   current_head ? 
   current_head-object.sha1 : NULL,
   0, !!current_head) ||
-   ref_transaction_commit(transaction, sb.buf, err)) {
+   ref_transaction_commit(transaction, sb.buf, err)) {
rollback_index_files();
die(_(HEAD: cannot update ref: %s), err);
}
diff --git a/builtin/replace.c b/builtin/replace.c
index cf0f56d..51e9ddf 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -162,7 +162,7 @@ static int replace_object(const char *object_ref, const 
char *replace_ref,
if (!transaction ||
ref_transaction_update(transaction, ref, repl, prev,
   0, !is_null_sha1(prev)) ||
-   ref_transaction_commit(transaction, NULL, err))
+   ref_transaction_commit(transaction, NULL, err))
  die(_(%s: failed to replace ref: %s), ref, err);
 
return 0;
diff --git a/builtin/tag.c b/builtin/tag.c
index dd53fb4..60b57a1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -646,7 +646,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (!transaction ||
ref_transaction_update(transaction, ref.buf, object, prev,
   0, !is_null_sha1(prev)) ||
-   ref_transaction_commit(transaction, NULL, err))
+   ref_transaction_commit(transaction, NULL, err))
  die(_(%s: cannot update the ref: %s), ref.buf, err);
if (force  !is_null_sha1(prev)  hashcmp(prev, object))
printf(_(Updated tag '%s' (was %s)\n), tag, 
find_unique_abbrev(prev, DEFAULT_ABBREV));
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a600ab3..4a0901d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -373,7 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
-   if (ref_transaction_commit(transaction, msg, err))
+   if (ref_transaction_commit(transaction, msg, err))
die(update_ref failed: %s, err);
return 0;
}
diff --git a/fast-import.c b/fast-import.c
index a2b05fa..3ce2f47 100644
--- a/fast-import.c
+++

[PATCH v3 06/19] refs.c: make update_ref_write to return error string on failure

Change update_ref_write to also return an error string on failure.
This makes the error avaialbel to ref_transaction_commit callers if the
transaction failed dur to update_ref_sha1/write_ref_sha1 failures.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 9d9eab8..7562151 100644
--- a/refs.c
+++ b/refs.c
@@ -3253,10 +3253,14 @@ static struct ref_lock *update_ref_lock(const char 
*refname,
 
 static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
-   enum action_on_err onerr)
+   char **err, enum action_on_err onerr)
 {
if (write_ref_sha1(lock, sha1, action)  0) {
const char *str = Cannot update the ref '%s'.;
+   if (err) {
+   *err = xmalloc(PATH_MAX + 26);
+   snprintf(*err, PATH_MAX + 26, str, refname);
+   }
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
@@ -3382,7 +3386,7 @@ int update_ref(const char *action, const char *refname,
lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
if (!lock)
return 1;
-   return update_ref_write(action, refname, sha1, lock, onerr);
+   return update_ref_write(action, refname, sha1, lock, NULL, onerr);
 }
 
 static int ref_update_compare(const void *r1, const void *r2)
@@ -3472,7 +3476,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
ret = update_ref_write(msg,
   update-refname,
   update-new_sha1,
-  update-lock, onerr);
+  update-lock, err, onerr);
update-lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 00/19] Use ref transactions from most callers

This patch series changes most of the places where the ref functions for
locking and writing refs to instead use the new ref transaction API. There
are still three more places where write_ref_sha1() is called from outside
of refs.c but those all will require more complex work and review so those
changes belong in a different patch series.

I think I have covered all issues raised on the previous reviews and also
done a bunch of cleanups and improvements to the transaction API.


Version 3:
 - Remove the walker patch for now. Walker needs more complex solution
   so defer it until the basics are done.
 - Remove the onerr argument to ref_transaction_commit(). All callers
   that need to die() on error now have to do this explicitely.
 - Pass an error string from ref_transaction_commit() back to the callers
   so that they can craft a nice error message upon failures.
 - Make ref_transaction_rollback() accept NULL as argument.
 - Change ref_transaction_commit() to take a pointer to pointer argument for
   the transaction and have it clear the callers pointer to NULL when
   invoked. This allows for much nicer handling of transaction rollback on
   failure.

Version 2:
 - Add a patch to ref_transaction_commit to make it honor onerr even if the
   error triggered in ref_Transaction_commit itself rather than in a call
   to other functions (that already honor onerr).
 - Add a patch to make the update_ref() helper function use transactions
   internally.
 - Change ref_transaction_update to die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change ref_transaction_create to die() instead of error() if new_sha1
   is false but we pass it a null_sha1.
 - Change ref_transaction_delete die() instead of error() if we pass
   if a NULL old_sha1 but have have_old == true.
 - Change several places to do  if(!transaction || ref_transaction_update()
   || ref_Transaction_commit()) die(generic-message) instead of checking each
   step separately and having a different message for each failure.
   Most users are likely not interested in what step of the transaction
   failed and only whether it failed or not.
 - Change commit.c to only pass a pointer to ref_transaction_update
   iff current_head is non-NULL.
   The previous patch used to compute a garbage pointer for
   current_head-object.sha1 and relied on the fact that ref_transaction_update
   would not try to dereference this pointer if !!current_head was 0.
 - Updated commit message for the walker_fetch change to try to justify why
   the change in locking semantics should not be harmful.

Ronnie Sahlberg (19):
  refs.c: constify the sha arguments for
ref_transaction_create|delete|update
  refs.c: allow passing NULL to ref_transaction_free
  refs.c: make ref_transaction_commit return an error string
  refs.c: return error string from ref_update_reject_duplicates on
failure
  update-ref.c: use the error string from _commit to print better
message
  refs.c: make update_ref_write to return error string on failure
  refs.c: remove the onerr argument to ref_transaction_commit
  refs.c: change ref_transaction_update() to do error checking and
return status
  refs.c: change ref_transaction_create to do error checking and return
status
  refs.c: ref_transaction_delete to check for error and return status
  tag.c: use ref transactions when doing updates
  replace.c: use the ref transaction functions for updates
  commit.c: use ref transactions for updates
  sequencer.c: use ref transactions for all ref updates
  fast-import.c: change update_branch to use ref transactions
  branch.c: use ref transaction for all ref updates
  refs.c: change update_ref to use a transaction
  refs.c: free the transaction before returning when number of updates
is 0
  refs.c: pass **transaction to commit and have it clear the pointer

 branch.c  |  39 ++--
 builtin/commit.c  |  23 +-
 builtin/replace.c |  14 +++---
 builtin/tag.c |  14 +++---
 builtin/update-ref.c  |  30 -
 fast-import.c |  18 
 refs.c| 122 +++---
 refs.h|  32 +++--
 sequencer.c   |  21 ++---
 t/t1400-update-ref.sh |  16 +++
 10 files changed, 206 insertions(+), 123 deletions(-)

-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 17/19] refs.c: change update_ref to use a transaction

Change the update_ref helper function to use a ref transaction internally.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 23 +++
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 1557c3c..95df4a9 100644
--- a/refs.c
+++ b/refs.c
@@ -3397,11 +3397,26 @@ int update_ref(const char *action, const char *refname,
   const unsigned char *sha1, const unsigned char *oldval,
   int flags, enum action_on_err onerr)
 {
-   struct ref_lock *lock;
-   lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
-   if (!lock)
+   struct ref_transaction *t;
+   char *err = NULL;
+
+   t = ref_transaction_begin();
+   if ((!t ||
+   ref_transaction_update(t, refname, sha1, oldval, flags,
+  !!oldval)) ||
+   (ref_transaction_commit(t, action, err)  !(t = NULL))) {
+const char *str = update_ref failed for ref '%s': %s;
+
+   ref_transaction_rollback(t);
+   switch (onerr) {
+   case UPDATE_REFS_MSG_ON_ERR: error(str, refname, err); break;
+   case UPDATE_REFS_DIE_ON_ERR: die(str, refname, err); break;
+   case UPDATE_REFS_QUIET_ON_ERR: break;
+   free(err);
+   }
return 1;
-   return update_ref_write(action, refname, sha1, lock, NULL, onerr);
+   }
+   return 0;
 }
 
 static int ref_update_compare(const void *r1, const void *r2)
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 18/19] refs.c: free the transaction before returning when number of updates is 0

We have to free the transaction before returning in the early check for
'return early if number of updates == 0' or else the following code would
create a memory leak with the transaction never being freed :
   t = ref_transaction_begin()
   ref_transaction_commit(t)

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 95df4a9..ffa9c83 100644
--- a/refs.c
+++ b/refs.c
@@ -3452,8 +3452,10 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
int n = transaction-nr;
struct ref_update **updates = transaction-updates;
 
-   if (!n)
+   if (!n) {
+   ref_transaction_free(transaction);
return 0;
+   }
 
if (err)
*err = NULL;
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 04/19] refs.c: return error string from ref_update_reject_duplicates on failure

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 0712912..9d9eab8 100644
--- a/refs.c
+++ b/refs.c
@@ -3393,6 +3393,7 @@ static int ref_update_compare(const void *r1, const void 
*r2)
 }
 
 static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+   char **err,
enum action_on_err onerr)
 {
int i;
@@ -3400,6 +3401,11 @@ static int ref_update_reject_duplicates(struct 
ref_update **updates, int n,
if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
const char *str =
Multiple updates for ref '%s' not allowed.;
+   if (err) {
+   *err = xmalloc(PATH_MAX + 41);
+   snprintf(*err, PATH_MAX + 41, str,
+updates[i]-refname);
+   }
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
error(str, updates[i]-refname); break;
@@ -3433,7 +3439,7 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
 
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
-   ret = ref_update_reject_duplicates(updates, n, onerr);
+   ret = ref_update_reject_duplicates(updates, n, err, onerr);
if (ret)
goto cleanup;
 
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 10/19] refs.c: ref_transaction_delete to check for error and return status

Change ref_transaction_delete() to do basic error checking and return
status. Update all callers to check the return for ref_transaction_delete()

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c |  5 +++--
 refs.c   | 15 ++-
 refs.h   |  8 
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 5f7dfc1..a600ab3 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -257,8 +257,9 @@ static const char *parse_cmd_delete(struct strbuf *input, 
const char *next)
if (*next != line_termination)
die(delete %s: extra input: %s, refname, next);
 
-   ref_transaction_delete(transaction, refname, old_sha1,
-  update_flags, have_old);
+   if (ref_transaction_delete(transaction, refname, old_sha1,
+  update_flags, have_old))
+   die(failed transaction delete for %s, refname);
 
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 762be85..1557c3c 100644
--- a/refs.c
+++ b/refs.c
@@ -3373,19 +3373,24 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
return 0;
 }
 
-void ref_transaction_delete(struct ref_transaction *transaction,
-   const char *refname,
-   const unsigned char *old_sha1,
-   int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *old_sha1,
+  int flags, int have_old)
 {
-   struct ref_update *update = add_update(transaction, refname);
+   struct ref_update *update;
 
+   if (have_old  !old_sha1)
+   die(have_old is true but old_sha1 is NULL);
+
+   update = add_update(transaction, refname);
update-flags = flags;
update-have_old = have_old;
if (have_old) {
assert(!is_null_sha1(old_sha1));
hashcpy(update-old_sha1, old_sha1);
}
+   return 0;
 }
 
 int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index e984267..8135131 100644
--- a/refs.h
+++ b/refs.h
@@ -259,10 +259,10 @@ int ref_transaction_create(struct ref_transaction 
*transaction,
  * old_sha1 holds the value that the reference should have had before
  * the update (which must not be the null SHA-1).
  */
-void ref_transaction_delete(struct ref_transaction *transaction,
-   const char *refname,
-   const unsigned char *old_sha1,
-   int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *old_sha1,
+  int flags, int have_old);
 
 /*
  * Commit all of the changes that have been queued in transaction, as
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 15/19] fast-import.c: change update_branch to use ref transactions

Change update_branch() to use ref transactions for updates.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 fast-import.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index fb4738d..a2b05fa 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1678,36 +1678,39 @@ found_entry:
 static int update_branch(struct branch *b)
 {
static const char *msg = fast-import;
-   struct ref_lock *lock;
+   struct ref_transaction *transaction;
unsigned char old_sha1[20];
+   char *err = NULL;
 
if (is_null_sha1(b-sha1))
return 0;
if (read_ref(b-name, old_sha1))
hashclr(old_sha1);
-   lock = lock_any_ref_for_update(b-name, old_sha1, 0, NULL);
-   if (!lock)
-   return error(Unable to lock %s, b-name);
if (!force_update  !is_null_sha1(old_sha1)) {
struct commit *old_cmit, *new_cmit;
 
old_cmit = lookup_commit_reference_gently(old_sha1, 0);
new_cmit = lookup_commit_reference_gently(b-sha1, 0);
if (!old_cmit || !new_cmit) {
-   unlock_ref(lock);
return error(Branch %s is missing commits., b-name);
}
 
if (!in_merge_bases(old_cmit, new_cmit)) {
-   unlock_ref(lock);
warning(Not updating %s
 (new tip %s does not contain %s),
b-name, sha1_to_hex(b-sha1), 
sha1_to_hex(old_sha1));
return -1;
}
}
-   if (write_ref_sha1(lock, b-sha1, msg)  0)
-   return error(Unable to update %s, b-name);
+   transaction = ref_transaction_begin();
+   if ((!transaction ||
+   ref_transaction_update(transaction, b-name, b-sha1, old_sha1,
+  0, 1)) ||
+   (ref_transaction_commit(transaction, msg, err) 
+!(transaction = NULL))) {
+   ref_transaction_rollback(transaction);
+   return error(Unable to update branch %s: %s, b-name, err);
+   }
return 0;
 }
 
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 13/19] commit.c: use ref transactions for updates

Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/commit.c | 23 ++-
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d9550c5..7e4c306 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
-   struct ref_lock *ref_lock;
struct commit_list *parents = NULL, **pptr = parents;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
+   struct ref_transaction *transaction;
+   char *err = NULL;
 
if (argc == 2  !strcmp(argv[1], -h))
usage_with_options(builtin_commit_usage, 
builtin_commit_options);
@@ -1667,12 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_release(author_ident);
free_commit_extra_headers(extra);
 
-   ref_lock = lock_any_ref_for_update(HEAD,
-  !current_head
-  ? NULL
-  : current_head-object.sha1,
-  0, NULL);
-
nl = strchr(sb.buf, '\n');
if (nl)
strbuf_setlen(sb, nl + 1 - sb.buf);
@@ -1681,13 +1676,15 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(sb, strlen(reflog_msg), : , 2);
 
-   if (!ref_lock) {
-   rollback_index_files();
-   die(_(cannot lock HEAD ref));
-   }
-   if (write_ref_sha1(ref_lock, sha1, sb.buf)  0) {
+   transaction = ref_transaction_begin();
+   if (!transaction ||
+   ref_transaction_update(transaction, HEAD, sha1,
+  current_head ? 
+  current_head-object.sha1 : NULL,
+  0, !!current_head) ||
+   ref_transaction_commit(transaction, sb.buf, err)) {
rollback_index_files();
-   die(_(cannot update HEAD ref));
+   die(_(HEAD: cannot update ref: %s), err);
}
 
unlink(git_path(CHERRY_PICK_HEAD));
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 14/19] sequencer.c: use ref transactions for all ref updates

Change to use ref transactions for all updates to refs.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 sequencer.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bde5f04..7d59f58 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,19 +272,29 @@ static int error_dirty_index(struct replay_opts *opts)
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
 {
-   struct ref_lock *ref_lock;
+   struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
-   int ret;
+   char *err = NULL;
 
read_cache();
if (checkout_fast_forward(from, to, 1))
exit(1); /* the callee should have complained already */
-   ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
-  0, NULL);
+
strbuf_addf(sb, %s: fast-forward, action_name(opts));
-   ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+   transaction = ref_transaction_begin();
+   if ((!transaction ||
+   ref_transaction_update(transaction, HEAD, to, from,
+  0, !unborn)) ||
+   (ref_transaction_commit(transaction, sb.buf, err) 
+!(transaction = NULL))) {
+   ref_transaction_rollback(transaction);
+   strbuf_release(sb);
+   return error(_(HEAD: Could not fast-forward: %s\n), err);
+   }
+
strbuf_release(sb);
-   return ret;
+   return 0;
 }
 
 static int do_recursive_merge(struct commit *base, struct commit *next,
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 01/19] refs.c: constify the sha arguments for ref_transaction_create|delete|update

ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.

Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 7 ---
 refs.h | 7 ---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 728a761..138ab70 100644
--- a/refs.c
+++ b/refs.c
@@ -3329,7 +3329,8 @@ static struct ref_update *add_update(struct 
ref_transaction *transaction,
 
 void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *new_sha1, unsigned char *old_sha1,
+   const unsigned char *new_sha1,
+   const unsigned char *old_sha1,
int flags, int have_old)
 {
struct ref_update *update = add_update(transaction, refname);
@@ -3343,7 +3344,7 @@ void ref_transaction_update(struct ref_transaction 
*transaction,
 
 void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *new_sha1,
+   const unsigned char *new_sha1,
int flags)
 {
struct ref_update *update = add_update(transaction, refname);
@@ -3357,7 +3358,7 @@ void ref_transaction_create(struct ref_transaction 
*transaction,
 
 void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *old_sha1,
+   const unsigned char *old_sha1,
int flags, int have_old)
 {
struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index 0f08def..892c5b6 100644
--- a/refs.h
+++ b/refs.h
@@ -239,7 +239,8 @@ void ref_transaction_rollback(struct ref_transaction 
*transaction);
  */
 void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *new_sha1, unsigned char *old_sha1,
+   const unsigned char *new_sha1,
+   const unsigned char *old_sha1,
int flags, int have_old);
 
 /*
@@ -250,7 +251,7 @@ void ref_transaction_update(struct ref_transaction 
*transaction,
  */
 void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *new_sha1,
+   const unsigned char *new_sha1,
int flags);
 
 /*
@@ -260,7 +261,7 @@ void ref_transaction_create(struct ref_transaction 
*transaction,
  */
 void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
-   unsigned char *old_sha1,
+   const unsigned char *old_sha1,
int flags, int have_old);
 
 /*
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 16/19] branch.c: use ref transaction for all ref updates

Change create_branch to use a ref transaction when creating the new branch.
ref_transaction_create will check that the ref does not already exist and fail
otherwise meaning that we no longer need to keep a lock on the ref during the
setup_tracking. This simplifies the code since we can now do the transaction
in one single step.

If the forcing flag is false then use ref_transaction_create since this will
fail if the ref already exist. Otherwise use ref_transaction_update.

This also fixes a race condition in the old code where two concurrent
create_branch could race since the lock_any_ref_for_update/write_ref_sha1
did not protect against the ref already existsing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 branch.c | 39 +--
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/branch.c b/branch.c
index 660097b..23cde1e 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,6 @@ void create_branch(const char *head,
   int force, int reflog, int clobber_head,
   int quiet, enum branch_track track)
 {
-   struct ref_lock *lock = NULL;
struct commit *commit;
unsigned char sha1[20];
char *real_ref, msg[PATH_MAX + 20];
@@ -285,15 +284,6 @@ void create_branch(const char *head,
die(_(Not a valid branch point: '%s'.), start_name);
hashcpy(sha1, commit-object.sha1);
 
-   if (!dont_change_ref) {
-   lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
-   if (!lock)
-   die_errno(_(Failed to lock ref for update));
-   }
-
-   if (reflog)
-   log_all_ref_updates = 1;
-
if (forcing)
snprintf(msg, sizeof msg, branch: Reset to %s,
 start_name);
@@ -301,13 +291,34 @@ void create_branch(const char *head,
snprintf(msg, sizeof msg, branch: Created from %s,
 start_name);
 
+   if (reflog)
+   log_all_ref_updates = 1;
+
+   if (!dont_change_ref) {
+   struct ref_transaction *transaction;
+   char *err = NULL;
+
+   transaction = ref_transaction_begin();
+   if (forcing) {
+   if (!transaction ||
+   ref_transaction_update(transaction, ref.buf, sha1,
+  NULL, 0, 0) ||
+   ref_transaction_commit(transaction, msg, err))
+ die_errno(_(%s: failed to write ref: %s),
+   ref.buf, err);
+   } else {
+   if (!transaction ||
+   ref_transaction_create(transaction, ref.buf, sha1,
+  0) ||
+   ref_transaction_commit(transaction, msg, err))
+ die_errno(_(%s: failed to write ref: %s),
+   ref.buf, err);
+   }
+   }
+
if (real_ref  track)
setup_tracking(ref.buf + 11, real_ref, track, quiet);
 
-   if (!dont_change_ref)
-   if (write_ref_sha1(lock, sha1, msg)  0)
-   die_errno(_(Failed to write ref));
-
strbuf_release(ref);
free(real_ref);
 }
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message

Call ref_transaction_commit with QUIET_ON_ERR and use the error string
that is returned to print a better log message if/after the transaction
fails.

Update the tests to reflect that the log message is now slightly different
  fatal: update_ref failed: Cannot lock the ref 'some ref'
versus from the previous
  fatal: Cannot lock the ref 'some ref'

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c  | 12 +++-
 t/t1400-update-ref.sh | 16 
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aaa06aa..47c9b53 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+   char *err;
struct option options[] = {
OPT_STRING( 'm', NULL, msg, N_(reason), N_(reason of the 
update)),
OPT_BOOL('d', NULL, delete, N_(delete the reference)),
@@ -359,17 +360,18 @@ int cmd_update_ref(int argc, const char **argv, const 
char *prefix)
die(Refusing to perform update with empty message.);
 
if (read_stdin) {
-   int ret;
transaction = ref_transaction_begin();
-
+   if (!transaction)
+   die(failed to update refs);
if (delete || no_deref || argc  0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
update_refs_stdin();
-   ret = ref_transaction_commit(transaction, msg, NULL,
-UPDATE_REFS_DIE_ON_ERR);
-   return ret;
+   if (ref_transaction_commit(transaction, msg, err,
+  UPDATE_REFS_QUIET_ON_ERR))
+   die(update_ref failed: %s, err);
+   return 0;
}
 
if (end_null)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 48ccc4d..53ed0cb 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -453,7 +453,7 @@ test_expect_success 'stdin fails with duplicate refs' '
create $a $m
EOF
test_must_fail git update-ref --stdin stdin 2err 
-   grep fatal: Multiple updates for ref '''$a''' not allowed. err
+   grep fatal: update_ref failed: Multiple updates for ref '''$a''' 
not allowed. err
 '
 
 test_expect_success 'stdin create ref works' '
@@ -511,7 +511,7 @@ test_expect_success 'stdin create ref works with path with 
space to blob' '
 test_expect_success 'stdin update ref fails with wrong old value' '
echo update $c $m $m~1 stdin 
test_must_fail git update-ref --stdin stdin 2err 
-   grep fatal: Cannot lock the ref '''$c''' err 
+   grep fatal: update_ref failed: Cannot lock the ref '''$c''' err 
test_must_fail git rev-parse --verify -q $c
 '
 
@@ -547,7 +547,7 @@ test_expect_success 'stdin update ref works with right old 
value' '
 test_expect_success 'stdin delete ref fails with wrong old value' '
echo delete $a $m~1 stdin 
test_must_fail git update-ref --stdin stdin 2err 
-   grep fatal: Cannot lock the ref '''$a''' err 
+   grep fatal: update_ref failed: Cannot lock the ref '''$a''' err 
git rev-parse $m expect 
git rev-parse $a actual 
test_cmp expect actual
@@ -634,7 +634,7 @@ test_expect_success 'stdin update refs fails with wrong old 
value' '
update $c  ''
EOF
test_must_fail git update-ref --stdin stdin 2err 
-   grep fatal: Cannot lock the ref '''$c''' err 
+   grep fatal: update_ref failed: Cannot lock the ref '''$c''' err 
git rev-parse $m expect 
git rev-parse $a actual 
test_cmp expect actual 
@@ -807,7 +807,7 @@ test_expect_success 'stdin -z fails option with unknown 
name' '
 test_expect_success 'stdin -z fails with duplicate refs' '
printf $F create $a $m create $b $m create $a $m stdin 
test_must_fail git update-ref -z --stdin stdin 2err 
-   grep fatal: Multiple updates for ref '''$a''' not allowed. err
+   grep fatal: update_ref failed: Multiple updates for ref '''$a''' 
not allowed. err
 '
 
 test_expect_success 'stdin -z create ref works' '
@@ -847,7 +847,7 @@ test_expect_success 'stdin -z create ref works with path 
with space to blob' '
 test_expect_success 'stdin -z update ref fails with wrong old value' '
printf $F update $c $m $m~1 stdin 
test_must_fail git update-ref -z --stdin stdin 2err 
-   grep fatal: Cannot lock the ref '''$c''' err 
+   grep fatal: update_ref failed: Cannot lock the ref '''$c''' err 
test_must_fail git rev-parse --verify -q $c
 '
 
@@ -883,7 +883,7

[PATCH v3 09/19] refs.c: change ref_transaction_create to do error checking and return status

Do basic error checking in ref_transaction_create() and make it return
status. Update all callers to check the result of ref_transaction_create()

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c |  4 +++-
 refs.c   | 17 +++--
 refs.h   |  8 
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 13d094d..5f7dfc1 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -225,7 +225,9 @@ static const char *parse_cmd_create(struct strbuf *input, 
const char *next)
if (*next != line_termination)
die(create %s: extra input: %s, refname, next);
 
-   ref_transaction_create(transaction, refname, new_sha1, update_flags);
+   if (ref_transaction_create(transaction, refname, new_sha1,
+  update_flags))
+   die(failed transaction create for %s, refname);
 
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 65cf407..762be85 100644
--- a/refs.c
+++ b/refs.c
@@ -3354,18 +3354,23 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
return 0;
 }
 
-void ref_transaction_create(struct ref_transaction *transaction,
-   const char *refname,
-   const unsigned char *new_sha1,
-   int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *new_sha1,
+  int flags)
 {
-   struct ref_update *update = add_update(transaction, refname);
+   struct ref_update *update;
+
+   if (!new_sha1 || is_null_sha1(new_sha1))
+   die(create ref with null new_sha1);
+
+   update = add_update(transaction, refname);
 
-   assert(!is_null_sha1(new_sha1));
hashcpy(update-new_sha1, new_sha1);
hashclr(update-old_sha1);
update-flags = flags;
update-have_old = 1;
+   return 0;
 }
 
 void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 6a361d7..e984267 100644
--- a/refs.h
+++ b/refs.h
@@ -249,10 +249,10 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
  * null SHA-1.  It is verified that the reference does not exist
  * already.
  */
-void ref_transaction_create(struct ref_transaction *transaction,
-   const char *refname,
-   const unsigned char *new_sha1,
-   int flags);
+int ref_transaction_create(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *new_sha1,
+  int flags);
 
 /*
  * Add a reference deletion to transaction.  If have_old is true, then
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

David Kastrup wrote:
 Jonathan Nieder jrnie...@gmail.com writes:

 I probably missed a subtlety, but the above comment reminded me of
 some netiquette I think this list is starting to forget.  If I have
 misread it, please let me know and skip the rest of this message.
[...]
 On the git list, the rule is simple.  Feel free to grumble, but make
 sure there is some contribution in the same message.
[...]
 Uh, Javier was commenting on a number of concrete proposals regarding
 the subject line What is missing from Git v2.0 and quoted Felipe
 directly.  As you seem to have lost the context, let me requote the
 respective portion:

I hadn't lost the context, but thanks for a pointer.

[...]
 = Reject non-fast-forward pulls by default =
[...]
 The patches were sent, the issues were addressed, people agreed, and
 yet nothing happened.

 [3][4]

 [...]

 [3] http://thread.gmane.org/gmane.comp.version-control.git/240030
 [4] http://thread.gmane.org/gmane.comp.version-control.git/235981

Unfortunately Felipe's summary is incomplete.

My message was meant as something that could be made into a reference
for when similar questions of netiquette come up in the future (as for
example they do all the time with Felipe).  That meant I didn't give
as good advice for your particular case than I should have.

For this particular case, my thoughts are:

If you believe those patches should be applied, please re-send them
with a summary of changes you've made (if any) and your opinion on
any unaddressed comments from the review thread.

If you believe those patches should not be applied, wouldn't it be
better to reply to that thread to help in case someone wants to pick
up the patches and fix them?

On the other hand if you just want to blow off steam, I guess that's
fine too.  Just don't expect it to result in any patches being
applied, code quality improving, etc.

Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string

Let ref_transaction_commit return an optional error string that describes
the transaction failure. Start by returning the same error as update_ref_lock
returns, modulo the initial error:/fatal: preamble.

This will make it easier for callers to craft better error messages when
a transaction call fails.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c |  2 +-
 refs.c   | 12 +++-
 refs.h   |  5 -
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..aaa06aa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char 
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
-   ret = ref_transaction_commit(transaction, msg,
+   ret = ref_transaction_commit(transaction, msg, NULL,
 UPDATE_REFS_DIE_ON_ERR);
return ret;
}
diff --git a/refs.c b/refs.c
index 2d83ef6..0712912 100644
--- a/refs.c
+++ b/refs.c
@@ -3414,7 +3414,8 @@ static int ref_update_reject_duplicates(struct ref_update 
**updates, int n,
 }
 
 int ref_transaction_commit(struct ref_transaction *transaction,
-  const char *msg, enum action_on_err onerr)
+  const char *msg, char **err,
+  enum action_on_err onerr)
 {
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3424,6 +3425,9 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
if (!n)
return 0;
 
+   if (err)
+   *err = NULL;
+
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
 
@@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction 
*transaction,
   update-flags,
   update-type, onerr);
if (!update-lock) {
+   if (err) {
+   const char *str = Cannot lock the ref '%s'.;
+   *err = xmalloc(PATH_MAX + 24);
+   snprintf(*err, PATH_MAX + 24, str,
+update-refname);
+   }
ret = 1;
goto cleanup;
}
diff --git a/refs.h b/refs.h
index 892c5b6..615c73d 100644
--- a/refs.h
+++ b/refs.h
@@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction 
*transaction,
  * Commit all of the changes that have been queued in transaction, as
  * atomically as possible.  Return a nonzero value if there is a
  * problem.  The ref_transaction is freed by this function.
+ * If error is non-NULL it will return an error string that describes
+ * why a commit failed. This string must be free()ed by the caller.
  */
 int ref_transaction_commit(struct ref_transaction *transaction,
-  const char *msg, enum action_on_err onerr);
+  const char *msg, char **err,
+  enum action_on_err onerr);
 
 /** Lock a ref and then write its file */
 int update_ref(const char *action, const char *refname,
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 08/19] refs.c: change ref_transaction_update() to do error checking and return status

Update ref_transaction_update() do some basic error checking and return
true on error. Update all callers to check ref_transaction_update() for error.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 builtin/update-ref.c | 10 ++
 refs.c   |  9 +++--
 refs.h   | 10 +-
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index df3cd92..13d094d 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -197,8 +197,9 @@ static const char *parse_cmd_update(struct strbuf *input, 
const char *next)
if (*next != line_termination)
die(update %s: extra input: %s, refname, next);
 
-   ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-  update_flags, have_old);
+   if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+  update_flags, have_old))
+   die(update %s: failed, refname);
 
update_flags = 0;
free(refname);
@@ -286,8 +287,9 @@ static const char *parse_cmd_verify(struct strbuf *input, 
const char *next)
if (*next != line_termination)
die(verify %s: extra input: %s, refname, next);
 
-   ref_transaction_update(transaction, refname, new_sha1, old_sha1,
-  update_flags, have_old);
+   if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+  update_flags, have_old))
+   die(failed transaction update for %s, refname);
 
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 0e2df81..65cf407 100644
--- a/refs.c
+++ b/refs.c
@@ -3334,19 +3334,24 @@ static struct ref_update *add_update(struct 
ref_transaction *transaction,
return update;
 }
 
-void ref_transaction_update(struct ref_transaction *transaction,
+int ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
const unsigned char *new_sha1,
const unsigned char *old_sha1,
int flags, int have_old)
 {
-   struct ref_update *update = add_update(transaction, refname);
+   struct ref_update *update;
+
+   if (have_old  !old_sha1)
+   die(have_old is true but old_sha1 is NULL);
 
+   update = add_update(transaction, refname);
hashcpy(update-new_sha1, new_sha1);
update-flags = flags;
update-have_old = have_old;
if (have_old)
hashcpy(update-old_sha1, old_sha1);
+   return 0;
 }
 
 void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index f3db632..6a361d7 100644
--- a/refs.h
+++ b/refs.h
@@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction 
*transaction);
  * that the reference should have had before the update, or zeros if
  * it must not have existed beforehand.
  */
-void ref_transaction_update(struct ref_transaction *transaction,
-   const char *refname,
-   const unsigned char *new_sha1,
-   const unsigned char *old_sha1,
-   int flags, int have_old);
+int ref_transaction_update(struct ref_transaction *transaction,
+  const char *refname,
+  const unsigned char *new_sha1,
+  const unsigned char *old_sha1,
+  int flags, int have_old);
 
 /*
  * Add a reference creation to transaction.  new_sha1 is the value
-- 
1.9.1.521.g5dc89fa

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1

2014-04-25 Thread Richard Hansen

On 2014-04-25 03:37, Simon Oosthoek wrote:
 (though tbh, I think you'd have to be in an automated situation
 to check out a branch that is basically a command to hack your
 system, a human would probably figure it too cumbersome, or too
 fishy)

You can get in trouble by cloning a malicious repository and cding to
the resulting directory.  See:

https://github.com/richardhansen/clonepwn

for a (benign) demonstration.  (Note the name of the default branch in
that repository -- it's not master.)

 
 + # not needed anymore; keep user's
 + # environment clean
 + unset __git_ps1_upstream_name
 
 We already have a lot of stuff in the user's environment beginning
 with __git, so I don't think the unset is necessary.
 
 If people rely on the string being set in their scripts, it can be
 bad to remove it. But if it's new in this patch,

The variable is new.

 I don't see the need to keep it. Cruft is bad IMO.

Agreed, although I am willing to remove those three lines if that is the
collective preference.

-Richard
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

Theodore Ts'o wrote:
 On Fri, Apr 25, 2014 at 09:48:53AM +0200, Philippe Vaucher wrote:
  
  I agree. The stage area is a very important concept in git, why not
  talk git commands that refers to it? Then we could add flags like
  --new-files or --deleted-files for better granularity than the current
  --all flag.
 
 One caution: The term stage/staged is already a little overloaded.
 We generally use the word staged to refer to changes that are in the
 index, but the term stage as a noun generally refers to referencing
 the different versions of a file during a merge operation (cf git
 ls-files --stage).
 
  I think starting by documenting the issues is a good idea, maybe on a
  wiki, and start some draft of a proposed solution that would improve
  in an iterative process.
 
 And it would be nice if the issues were discussed in a way that acknowledged
 that all changes have tradeoffs, both positive and negative,

They have been discussed at length:

http://thread.gmane.org/gmane.comp.version-control.git/197111
http://thread.gmane.org/gmane.comp.version-control.git/166675
http://thread.gmane.org/gmane.comp.version-control.git/115666
http://thread.gmane.org/gmane.comp.version-control.git/236127

When I say literally everbody agreed to move away from the name index (except
Junio and another guy) I mean it. I even composed a list:

http://article.gmane.org/gmane.comp.version-control.git/233469

Jeff King, Jonathan Nieder, Matthieu Moy, they all agreed.

 or for people for whom English might not be the first language.

People whom English is not their first language also agreed index is a
terrible term.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

Philippe Vaucher wrote:
  Yes, of course there should be a list of both positive and negative
  tradeoffs. But I think the overloaded argument can be easily solved
  by renaming one of the overloads.
 
  And renaming one of a term also has costs, especially if it is one
  that is in use in large amounts of documentation, both in the git man
  pages, and in web pages across the web.
 
 It's just impossible to change terms and have previous documentation
 still be valid. Sure, you can list it in the cons section as an
 argument, but it's not very convincing in itself because it applies to
 pretty much any interface changes. I think the main idea is to
 _improve_ the interface, which means rename things so it is more
 consistent and so concepts are easier for new comers to grasp. We
 could still support old terms for a while and eventually deprecate
 them.

And that's exactly what I did in my patch series: start introducing the --stage
options along the current ones.

http://thread.gmane.org/gmane.comp.version-control.git/236127/focus=236128

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 01/14] Add proper 'stage' command

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-stage.txt| 46 +
 Makefile   |  2 +-
 builtin.h  |  1 +
 builtin/stage.c| 53 ++
 contrib/completion/git-completion.bash | 24 ++-
 git.c  |  2 +-
 6 files changed, 120 insertions(+), 8 deletions(-)
 create mode 100644 builtin/stage.c

diff --git a/Documentation/git-stage.txt b/Documentation/git-stage.txt
index ba3fe0d..5b42b29 100644
--- a/Documentation/git-stage.txt
+++ b/Documentation/git-stage.txt
@@ -3,20 +3,56 @@ git-stage(1)
 
 NAME
 
-git-stage - Add file contents to the staging area
+git-stage - manage the staging area
 
 
 SYNOPSIS
 
 [verse]
-'git stage' args...
-
+'git stage' [options] [--] [paths...]
+'git stage add' [options] [--] [paths...]
+'git stage reset' [-q|--patch] [--] [paths...]
+'git stage diff' [options] [commit] [--] [paths...]
+'git stage rm' [options] [--] [paths...]
+'git stage apply' [options] [--] [paths...]
 
 DESCRIPTION
 ---
 
-This is a synonym for linkgit:git-add[1].  Please refer to the
-documentation of that command.
+This command is useful to manage the staging area through other subcommands.
+
+COMMANDS
+
+
+With no arguments, it's a synonym for linkgit:git-add[1].
+
+'add'::
+
+Adds file contents to the staging area. See linkgit:git-add[1].
+
+'reset'::
+
+Resets the staging area. See linkgit:git-reset[1].
+
+'diff'::
+
+View the changes you staged for the next commit. See linkgit:git-diff[1] 
--staged.
+
+'rm'::
+
+Remove files from the staging area only. See linkgit:git-rm[1] --staged.
+
+'apply'::
+
+Apply a patch to the staging area. See linkgit:git-rm[1] --staged.
+
+SEE ALSO
+
+linkgit:git-add[1]
+linkgit:git-reset[1]
+linkgit:git-diff[1]
+linkgit:git-rm[1]
+linkgit:git-apply[1]
 
 GIT
 ---
diff --git a/Makefile b/Makefile
index dddaf4f..059cd50 100644
--- a/Makefile
+++ b/Makefile
@@ -589,7 +589,6 @@ BUILT_INS += git-fsck-objects$X
 BUILT_INS += git-init$X
 BUILT_INS += git-merge-subtree$X
 BUILT_INS += git-show$X
-BUILT_INS += git-stage$X
 BUILT_INS += git-status$X
 BUILT_INS += git-whatchanged$X
 
@@ -977,6 +976,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stage.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
 BUILTIN_OBJS += builtin/tag.o
diff --git a/builtin.h b/builtin.h
index c47c110..76b0a48 100644
--- a/builtin.h
+++ b/builtin.h
@@ -115,6 +115,7 @@ extern int cmd_send_pack(int argc, const char **argv, const 
char *prefix);
 extern int cmd_shortlog(int argc, const char **argv, const char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
+extern int cmd_stage(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stage.c b/builtin/stage.c
new file mode 100644
index 000..7c4d442
--- /dev/null
+++ b/builtin/stage.c
@@ -0,0 +1,53 @@
+/*
+ * 'git stage' builtin command
+ *
+ * Copyright (C) 2013 Felipe Contreras
+ */
+
+#include builtin.h
+#include parse-options.h
+
+static const char *const stage_usage[] = {
+   N_(git stage [options] [--] paths...),
+   N_(git stage add [options] [--] paths...),
+   N_(git stage apply [options] [patch...]),
+   N_(git stage reset [-q|--patch] [--] paths...),
+   N_(git stage diff [options] [commit] [--] paths...),
+   N_(git stage rm [options] [--] paths...),
+   NULL
+};
+
+int cmd_stage(int argc, const char **argv, const char *prefix)
+{
+   struct option options[] = { OPT_END() };
+
+   argc = parse_options(argc, argv, prefix, options, stage_usage,
+   PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN | 
PARSE_OPT_KEEP_DASHDASH);
+
+   if (argc  1) {
+   if (!strcmp(argv[1], add))
+   return cmd_add(argc - 1, argv + 1, prefix);
+   if (!strcmp(argv[1], reset))
+   return cmd_reset(argc - 1, argv + 1, prefix);
+   if (!strcmp(argv[1], diff)) {
+   argv[0] = diff;
+   argv[1] = --staged;
+
+   return cmd_diff(argc, argv, prefix);
+   }
+   if (!strcmp(argv[1], rm)) {
+   argv[0] = rm;
+   argv[1] = --cached;
+
+   return cmd_rm(argc, argv, prefix);
+   }
+   if (!strcmp(argv[1], apply)) {
+

[PATCH v2 try2 03/14] diff: document --staged

Synonym for --cached.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-diff.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 56fb7e5..ca2a0ed 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git diff' [options] [commit] [--] [path...]
-'git diff' [options] --cached [commit] [--] [path...]
+'git diff' [options] [--cached|--staged] [commit] [--] [path...]
 'git diff' [options] commit commit [--] [path...]
 'git diff' [options] blob blob
 'git diff' [options] [--no-index] [--] path path
@@ -38,7 +38,7 @@ two blob objects, or changes between two files on disk.
or when running the command outside a working tree
controlled by Git.
 
-'git diff' [--options] --cached [commit] [--] [path...]::
+'git diff' [--options] [--cached|--staged] [commit] [--] [path...]::
 
This form is to view the changes you staged for the next
commit relative to the named commit.  Typically you
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 00/14] Officially start moving to the term 'staging area'

tl;dr: everyone except Junio C Hamano and Drew Northup agrees; we should move
away from the name the index.

It has been discussed many times in the past that 'index' is not an
appropriate description for what the high-level user does with it, and
it has been agreed that 'staging area' is the best term.

The term 'staging area' is more intuitive for newcomers which are more
familiar with English than with Git, and it seems to be a
straightforward mental notion for people with different mother tongues.

In fact it is so intuitive that it's used already in a lot online
documentation, and the people that do teach Git professionally use this
term, because it's easier for many kinds of audiences to grasp.

The meaning of the words 'cache' and 'index' doesn't represent correctly
the mental model of the high-level user:

cache: a 'cache' is a place for easier access; a squirrel caches nuts
so it doesn't have to go looking for them in the future when it might
be much more difficult. Git porcelain is not using the staging area
for easier future access; it's not a cache.

index: an 'index' is a guide of pointers to something else; a book
index has a list of entries so the reader can locate information
easily without having to go through the whole book. Git porcelain is
not using the staging area to find out entries quicker; it's not an
index.

stage: a 'stage' is a special area designated for convenience in order
for some activity to take place; an orator would prepare a stage in
order for her speak to be successful, otherwise many people might not
be able to hear, or see her. Git porcelain is using the staging area
precisely as a special area to be separated from the working directory
for convenience.

The term 'stage' is a good noun itself, but also 'staging area', it
has a good verb; 'to stage', and a nice past-participle; 'staged'.

The first step in moving Git towards this term, is first to add --stage
options for every command that uses --index or --cache. However, there's
a problem with the 'git apply' command, because it treats --index and
--cache differently. Different solutions were proposed, including a
special --stage-only option, however, I think the best solution is a
--[no-]work option to specify if the working directory should be touched
or not, so --index becomes --staged, and --cached becomes --staged
--no-work.

In addition, the 'git stage' command can be extended so the staging area
can be brought closer to the user, like other important Git concepts,
like 'git branch, 'git tag', and 'git remote'. For example, the command
'git stage edit' (which allows the user to edit directly the diff from
HEAD to the staging area) can have a home, where previously there was no
place. It would become natural then to do 'git stage diff', and then
'git stage edit' (to edit the previous diff).

After adding the new --stage options and making sure no functionality is
lost, they can become the recommended ones in the documentation,
eventually, the old ones get deprecated, and eventually obsoleted.

Also, the documentation would need to be updated to replace many
instances of 'the index', with 'the staging area' in porcelain commands.

Moreover, the --stage and --work options also make sense for 'git
reset', and after these options are added, the complicated table to
explain the different behaviors between --soft, --mixed, and --hard
becomes so simple it's not needed any more:

  working stage HEAD target working stage HEAD
  
   A   B CD --no-stage  A   B D
--stage A   D D
--work  D   D D

  working stage HEAD target working stage HEAD
  
   A   B CC --no-stage  A   B C
--stage A   C C
--work  C   C C

  working stage HEAD target working stage HEAD
  
   B   B CD --no-stage  B   B D
--stage B   D D
--work  D   D D

  working stage HEAD target working stage HEAD
  
   B   B CC --no-stage  B   B C
--stage B   C C
--work  C   C C

  working stage HEAD target working stage HEAD
  
   B   C CD --no-stage  B   C D
--stage B   D D
--work  D   D D

  working stage HEAD target working

[PATCH v2 try2 13/14] reset: allow --keep with --stage

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-reset.txt |  2 +-
 builtin/reset.c | 13 ++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 5cd75a8..a1419c9 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git reset' [-q] [tree-ish] [--] paths...
 'git reset' (--patch | -p) [tree-ish] [--] [paths...]
 'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [commit]
-'git reset' [--stage | --work] [-q] [commit]
+'git reset' [--stage | --work | --keep] [-q] [commit]
 
 DESCRIPTION
 ---
diff --git a/builtin/reset.c b/builtin/reset.c
index c40987e..8d6d9a1 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -23,7 +23,7 @@
 
 static const char * const git_reset_usage[] = {
N_(git reset [--mixed | --soft | --hard | --merge | --keep] [-q] 
[commit]),
-   N_(git reset [--stage | --work] [-q] [commit]),
+   N_(git reset [--stage | --work | --keep] [-q] [commit]),
N_(git reset [-q] tree-ish [--] paths...),
N_(git reset --patch [tree-ish] [--] [paths...]),
NULL
@@ -306,8 +306,15 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
}
 
if (stage = 0 || working_tree = 0) {
-   if (reset_type != NONE)
+   int keep = 0;
+
+   if (reset_type == KEEP) {
+   if (working_tree == 1)
+   die(_(--keep is incompatible with --work));
+   keep = 1;
+   } else if (reset_type != NONE) {
die(_(--{stage,work} are incompatible with 
--{hard,mixed,soft,merge}));
+   }
 
if (working_tree == 1) {
if (stage == 0)
@@ -315,7 +322,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
reset_type = HARD;
} else {
if (stage == 1)
-   reset_type = NONE;
+   reset_type = keep ? KEEP : NONE;
else
reset_type = SOFT;
}
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 12/14] reset: add --stage and --work options

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-reset.txt |  8 
 builtin/reset.c | 20 
 2 files changed, 28 insertions(+)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index f445cb3..5cd75a8 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 'git reset' [-q] [tree-ish] [--] paths...
 'git reset' (--patch | -p) [tree-ish] [--] [paths...]
 'git reset' [--soft | --mixed | --hard | --merge | --keep] [-q] [commit]
+'git reset' [--stage | --work] [-q] [commit]
 
 DESCRIPTION
 ---
@@ -81,6 +82,13 @@ but carries forward unmerged index entries.
different between commit and HEAD.
If a file that is different between commit and HEAD has local changes,
reset is aborted.
+
+--stage::
+   Reset the index, basically `--mixed`. `--no-stage` is the equivalent of
+   `--soft`.
+
+--work::
+   Resets the working tree, basically `--hard`.
 --
 
 If you want to undo a commit other than the latest on a branch,
diff --git a/builtin/reset.c b/builtin/reset.c
index a991344..c40987e 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -23,6 +23,7 @@
 
 static const char * const git_reset_usage[] = {
N_(git reset [--mixed | --soft | --hard | --merge | --keep] [-q] 
[commit]),
+   N_(git reset [--stage | --work] [-q] [commit]),
N_(git reset [-q] tree-ish [--] paths...),
N_(git reset --patch [tree-ish] [--] [paths...]),
NULL
@@ -254,6 +255,7 @@ static int reset_refs(const char *rev, const unsigned char 
*sha1)
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
int reset_type = NONE, update_ref_status = 0, quiet = 0;
+   int stage = -1, working_tree = -1;
int patch_mode = 0, unborn;
const char *rev;
unsigned char sha1[20];
@@ -269,6 +271,8 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
N_(reset HEAD, index and working tree), 
MERGE),
OPT_SET_INT(0, keep, reset_type,
N_(reset HEAD but keep local changes), KEEP),
+   OPT_BOOL(0, stage, stage, N_(reset index)),
+   OPT_BOOL(0, work, working_tree, N_(reset working tree)),
OPT_BOOL('p', patch, patch_mode, N_(select hunks 
interactively)),
OPT_END()
};
@@ -301,6 +305,22 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
hashcpy(sha1, tree-object.sha1);
}
 
+   if (stage = 0 || working_tree = 0) {
+   if (reset_type != NONE)
+   die(_(--{stage,work} are incompatible with 
--{hard,mixed,soft,merge}));
+
+   if (working_tree == 1) {
+   if (stage == 0)
+   die(_(--no-stage doesn't make sense with 
--work));
+   reset_type = HARD;
+   } else {
+   if (stage == 1)
+   reset_type = NONE;
+   else
+   reset_type = SOFT;
+   }
+   }
+
if (patch_mode) {
if (reset_type != NONE)
die(_(--patch is incompatible with 
--{hard,mixed,soft}));
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 14/14] completion: update 'git reset' new stage options

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/completion/git-completion.bash | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 52d83f2..e9b793b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2248,7 +2248,8 @@ _git_reset ()
 
case $cur in
--*)
-   __gitcomp --merge --mixed --hard --soft --patch
+   __gitcomp --merge --mixed --hard --soft --patch --keep --merge
+   --stage --no-stage --work --no-work
return
;;
esac
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 07/14] stash: add --stage to pop and apply

Synonym of --index.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-stash.txt | 8 
 git-stash.sh| 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 21a01c2..5fdaa35 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git stash' list [options]
 'git stash' show [stash]
 'git stash' drop [-q|--quiet] [stash]
-'git stash' ( pop | apply ) [--index] [-q|--quiet] [stash]
+'git stash' ( pop | apply ) [--index|--stage] [-q|--quiet] [stash]
 'git stash' branch branchname [stash]
 'git stash' [save [-p|--patch] [-k|--[no-]keep-index|--[no-]stage] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] [message]]
@@ -96,7 +96,7 @@ show [stash]::
it will accept any format known to 'git diff' (e.g., `git stash show
-p stash@{1}` to view the second most recent stash in patch form).
 
-pop [--index] [-q|--quiet] [stash]::
+pop [--index|--stage] [-q|--quiet] [stash]::
 
Remove a single stashed state from the stash list and apply it
on top of the current working tree state, i.e., do the inverse
@@ -110,12 +110,12 @@ and call `git stash drop` manually afterwards.
 If the `--index` option is used, then tries to reinstate not only the working
 tree's changes, but also the index's ones. However, this can fail, when you
 have conflicts (which are stored in the index, where you therefore can no
-longer apply the changes as they were originally).
+longer apply the changes as they were originally). `--stage` is a synonym.
 +
 When no `stash` is given, `stash@{0}` is assumed, otherwise `stash` must
 be a reference of the form `stash@{revision}`.
 
-apply [--index] [-q|--quiet] [stash]::
+apply [--index|--stage] [-q|--quiet] [stash]::
 
Like `pop`, but do not remove the state from the stash list. Unlike 
`pop`,
`stash` may be any commit that looks like a commit created by
diff --git a/git-stash.sh b/git-stash.sh
index bff4ecc..0f764fe 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -5,7 +5,7 @@ dashless=$(basename $0 | sed -e 's/-/ /')
 USAGE=list [options]
or: $dashless show [stash]
or: $dashless drop [-q|--quiet] [stash]
-   or: $dashless ( pop | apply ) [--index] [-q|--quiet] [stash]
+   or: $dashless ( pop | apply ) [--index|--stage] [-q|--quiet] [stash]
or: $dashless branch branchname [stash]
or: $dashless [save [--patch] [-k|--[no-]keep-index|--[no-]stage] 
[-q|--quiet]
   [-u|--include-untracked] [-a|--all] [message]]
@@ -373,7 +373,7 @@ parse_flags_and_rev()
-q|--quiet)
GIT_QUIET=-t
;;
-   --index)
+   --index|--stage)
INDEX_OPTION=--index
;;
-*)
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 09/14] apply: add --stage option

Synonym for --index.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-apply.txt | 5 -
 builtin/apply.c | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index f605327..8c047ef 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -9,7 +9,7 @@ git-apply - Apply a patch to files and/or to the index
 SYNOPSIS
 
 [verse]
-'git apply' [--stat] [--numstat] [--summary] [--check] [--index] [--3way]
+'git apply' [--stat] [--numstat] [--summary] [--check] [--index|--stage] 
[--3way]
  [--apply] [--no-add] [--build-fake-ancestor=file] [-R | --reverse]
  [--allow-binary-replacement | --binary] [--reject] [-z]
  [-pn] [-Cn] [--inaccurate-eof] [--recount] [--cached]
@@ -67,6 +67,9 @@ OPTIONS
up-to-date, it is flagged as an error.  This flag also
causes the index file to be updated.
 
+--stage::
+   Synonym for --index.
+
 --cached::
Apply a patch without touching the working tree. Instead take the
cached data, apply the patch, and store the result in the index
diff --git a/builtin/apply.c b/builtin/apply.c
index b0d0986..a97363c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4377,6 +4377,8 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix_)
N_(instead of applying the patch, see if the patch is 
applicable)),
OPT_BOOL(0, index, check_index,
N_(make sure the patch is applicable to the current 
index)),
+   OPT_BOOL(0, stage, check_index,
+   N_(make sure the patch is applicable to the current 
index)),
OPT_BOOL(0, cached, cached,
N_(apply a patch without touching the working tree)),
OPT_BOOL(0, apply, force_apply,
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 11/14] completion: update --staged options

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 contrib/completion/git-completion.bash | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 2ec7b1a..52d83f2 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -889,7 +889,7 @@ _git_apply ()
__gitcomp 
--stat --numstat --summary --check --index
--cached --index-info --reverse --reject --unidiff-zero
-   --apply --no-add --exclude=
+   --apply --no-add --exclude= --staged
--ignore-whitespace --ignore-space-change
--whitespace= --inaccurate-eof --verbose

@@ -1302,7 +1302,7 @@ _git_grep ()
case $cur in
--*)
__gitcomp 
-   --cached
+   --cached --staged
--text --ignore-case --word-regexp --invert-match
--full-name --line-number
--extended-regexp --basic-regexp --fixed-strings
@@ -2270,7 +2270,7 @@ _git_rm ()
 {
case $cur in
--*)
-   __gitcomp --cached --dry-run --ignore-unmatch --quiet
+   __gitcomp --cached --staged --dry-run --ignore-unmatch --quiet
return
;;
esac
@@ -2337,7 +2337,7 @@ _git_show_branch ()
 
 _git_stash ()
 {
-   local save_opts='--keep-index --no-keep-index --quiet --patch'
+   local save_opts='--keep-index --no-keep-index --stage --no-stage 
--quiet --patch'
local subcommands='save list show apply clear drop pop create branch'
local subcommand=$(__git_find_on_cmdline $subcommands)
if [ -z $subcommand ]; then
@@ -2357,7 +2357,7 @@ _git_stash ()
__gitcomp $save_opts
;;
apply,--*|pop,--*)
-   __gitcomp --index --quiet
+   __gitcomp --index --stage --quiet
;;
show,--*|drop,--*|branch,--*)
;;
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 10/14] apply: add --work, --no-work options

'git apply', 'git apply --index', 'git apply --cached' do different
things, but what they do is not precisely clear, specially since no
other commands has similar distinctions.

With --no-work (--work being the default), it's clear what the option
would do; modify, or not, the working directory.

So, --work (the default), doesn't cause any changes, and --no-work
enables the current --cache if used with --index.

Eventually --work might replace --cache, if these options are
standarized in the whole git toolset.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-apply.txt | 6 +-
 builtin/apply.c | 5 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index 8c047ef..95f5485 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -16,7 +16,7 @@ SYNOPSIS
  [--ignore-space-change | --ignore-whitespace ]
  [--whitespace=(nowarn|warn|fix|error|error-all)]
  [--exclude=path] [--include=path] [--directory=root]
- [--verbose] [patch...]
+ [--verbose] [--no-work] [patch...]
 
 DESCRIPTION
 ---
@@ -75,6 +75,10 @@ OPTIONS
cached data, apply the patch, and store the result in the index
without using the working tree. This implies `--index`.
 
+--[no-]work::
+   Apply a patch with or without touching the working tree, essentially
+   `--no-work` plus `--index` are the equivalent of `--cached`.
+
 -3::
 --3way::
When the patch does not apply cleanly, fall back on 3-way merge if
diff --git a/builtin/apply.c b/builtin/apply.c
index a97363c..68cdef1 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4350,6 +4350,7 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix_)
int errs = 0;
int is_not_gitdir = !startup_info-have_repository;
int force_apply = 0;
+   int work = 1;
 
const char *whitespace_option = NULL;
 
@@ -4381,6 +4382,8 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix_)
N_(make sure the patch is applicable to the current 
index)),
OPT_BOOL(0, cached, cached,
N_(apply a patch without touching the working tree)),
+   OPT_BOOL(0, work, work,
+   N_(modify the working tree)),
OPT_BOOL(0, apply, force_apply,
N_(also apply the patch (use with 
--stat/--summary/--check))),
OPT_BOOL('3', 3way, threeway,
@@ -4433,6 +4436,8 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix_)
argc = parse_options(argc, argv, prefix, builtin_apply_options,
apply_usage, 0);
 
+   if (check_index  !work)
+   cached = 1;
if (apply_with_reject  threeway)
die(--reject and --3way cannot be used together.);
if (cached  threeway)
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 06/14] stash: add --stage option to save

--no-stage is synonym for --keep-index.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-stash.txt | 6 +++---
 git-stash.sh| 8 +++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index db7e803..21a01c2 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [stash]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [stash]
 'git stash' branch branchname [stash]
-'git stash' [save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+'git stash' [save [-p|--patch] [-k|--[no-]keep-index|--[no-]stage] [-q|--quiet]
 [-u|--include-untracked] [-a|--all] [message]]
 'git stash' clear
 'git stash' create [message]
@@ -44,7 +44,7 @@ is also possible).
 OPTIONS
 ---
 
-save [-p|--patch] [--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
[-q|--quiet] [message]::
+save [-p|--patch] [--[no-]keep-index|--[no-]stage] [-u|--include-untracked] 
[-a|--all] [-q|--quiet] [message]::
 
Save your local modifications to a new 'stash', and run `git reset
--hard` to revert them.  The message part is optional and gives
@@ -54,7 +54,7 @@ save [-p|--patch] [--[no-]keep-index] 
[-u|--include-untracked] [-a|--all] [-q|--
subcommand from making an unwanted stash.
 +
 If the `--keep-index` option is used, all changes already added to the
-index are left intact.
+index are left intact. Same with `--no-stage`, which is a synonym.
 +
 If the `--include-untracked` option is used, all untracked files are also
 stashed and then cleaned up with `git clean`, leaving the working directory
diff --git a/git-stash.sh b/git-stash.sh
index f0a94ab..bff4ecc 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,7 +7,7 @@ USAGE=list [options]
or: $dashless drop [-q|--quiet] [stash]
or: $dashless ( pop | apply ) [--index] [-q|--quiet] [stash]
or: $dashless branch branchname [stash]
-   or: $dashless [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]
+   or: $dashless [save [--patch] [-k|--[no-]keep-index|--[no-]stage] 
[-q|--quiet]
   [-u|--include-untracked] [-a|--all] [message]]
or: $dashless clear
 
@@ -204,6 +204,12 @@ save_stash () {
--no-keep-index)
keep_index=n
;;
+   --stage)
+   keep_index=n
+   ;;
+   --no-stage)
+   keep_index=t
+   ;;
-p|--patch)
patch_mode=t
# only default to keep if we don't already have an 
override
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 08/14] submodule: add --staged options

Synonym for --cached.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-submodule.txt |  8 ++--
 git-submodule.sh| 10 +-
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index bfef8a0..904e007 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -11,13 +11,13 @@ SYNOPSIS
 [verse]
 'git submodule' [--quiet] add [-b branch] [-f|--force] [--name name]
  [--reference repository] [--depth depth] [--] repository 
[path]
-'git submodule' [--quiet] status [--cached] [--recursive] [--] [path...]
+'git submodule' [--quiet] status [--cached|--staged] [--recursive] [--] 
[path...]
 'git submodule' [--quiet] init [--] [path...]
 'git submodule' [--quiet] deinit [-f|--force] [--] path...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
  [-f|--force] [--rebase] [--reference repository] [--depth 
depth]
  [--merge] [--recursive] [--] [path...]
-'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) n]
+'git submodule' [--quiet] summary [--cached|--staged|--files] 
[(-n|--summary-limit) n]
  [commit] [--] [path...]
 'git submodule' [--quiet] foreach [--recursive] command
 'git submodule' [--quiet] sync [--] [path...]
@@ -248,6 +248,10 @@ OPTIONS
commands typically use the commit found in the submodule HEAD, but
with this option, the commit stored in the index is used instead.
 
+
+--staged::
+   Synonym for `--cached`.
+
 --files::
This option is only valid for the summary command. This command
compares the commit in the index with that in the submodule HEAD
diff --git a/git-submodule.sh b/git-submodule.sh
index 4a30087..af6df79 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -6,11 +6,11 @@
 
 dashless=$(basename $0 | sed -e 's/-/ /')
 USAGE=[--quiet] add [-b branch] [-f|--force] [--name name] [--reference 
repository] [--] repository [path]
-   or: $dashless [--quiet] status [--cached] [--recursive] [--] [path...]
+   or: $dashless [--quiet] status [--cached|--staged] [--recursive] [--] 
[path...]
or: $dashless [--quiet] init [--] [path...]
or: $dashless [--quiet] deinit [-f|--force] [--] path...
or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--rebase] [--reference repository] [--merge] [--recursive] [--] 
[path...]
-   or: $dashless [--quiet] summary [--cached|--files] [--summary-limit n] 
[commit] [--] [path...]
+   or: $dashless [--quiet] summary [--cached|--staged|--files] 
[--summary-limit n] [commit] [--] [path...]
or: $dashless [--quiet] foreach [--recursive] command
or: $dashless [--quiet] sync [--recursive] [--] [path...]
 OPTIONS_SPEC=
@@ -995,7 +995,7 @@ cmd_summary() {
while test $# -ne 0
do
case $1 in
-   --cached)
+   --cached|--staged)
cached=$1
;;
--files)
@@ -1200,7 +1200,7 @@ cmd_status()
-q|--quiet)
GIT_QUIET=1
;;
-   --cached)
+   --cached|--staged)
cached=1
;;
--recursive)
@@ -1367,7 +1367,7 @@ do
esac
branch=$2; shift
;;
-   --cached)
+   --cached|--staged)
cached=$1
;;
--)
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 05/14] rm: add --staged option

Synonym for --cached.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-rm.txt | 5 -
 builtin/rm.c | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index f1efc11..458880b 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -8,7 +8,7 @@ git-rm - Remove files from the working tree and from the index
 SYNOPSIS
 
 [verse]
-'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] 
file...
+'git rm' [-f | --force] [-n] [-r] [--cached | --staged] [--ignore-unmatch] 
[--quiet] [--] file...
 
 DESCRIPTION
 ---
@@ -60,6 +60,9 @@ OPTIONS
Working tree files, whether modified or not, will be
left alone.
 
+--staged::
+   Synonym for --cached.
+
 --ignore-unmatch::
Exit with a zero status even if no files matched.
 
diff --git a/builtin/rm.c b/builtin/rm.c
index 0564218..657314e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -269,6 +269,7 @@ static struct option builtin_rm_options[] = {
OPT__DRY_RUN(show_only, N_(dry run)),
OPT__QUIET(quiet, N_(do not list removed files)),
OPT_BOOL( 0 , cached, index_only, N_(only remove from the 
index)),
+   OPT_BOOL( 0 , staged, index_only, N_(only remove from the 
index)),
OPT__FORCE(force, N_(override the up-to-date check)),
OPT_BOOL('r', NULL, recursive,  N_(allow recursive 
removal)),
OPT_BOOL( 0 , ignore-unmatch, ignore_unmatch,
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 04/14] grep: add --staged option

Synonym for --cached.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-grep.txt | 5 -
 builtin/grep.c | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index f837334..6ed84d7 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -25,7 +25,7 @@ SYNOPSIS
   [-W | --function-context]
   [-f file] [-e] pattern
   [--and|--or|--not|(|)|-e pattern...]
-  [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | 
tree...]
+  [ [--[no-]exclude-standard] [--cached | --staged | --no-index | 
--untracked] | tree...]
   [--] [pathspec...]
 
 DESCRIPTION
@@ -60,6 +60,9 @@ OPTIONS
Instead of searching tracked files in the working tree, search
blobs registered in the index file.
 
+--staged::
+   Synonym for `--cached`.
+
 --no-index::
Search files in the current directory that is not managed by Git.
 
diff --git a/builtin/grep.c b/builtin/grep.c
index 69ac2d8..dc2edaf 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -638,6 +638,8 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
struct option options[] = {
OPT_BOOL(0, cached, cached,
N_(search in index instead of in the work tree)),
+   OPT_BOOL(0, staged, cached,
+   N_(search in index instead of in the work tree)),
OPT_NEGBIT(0, no-index, use_index,
 N_(find in contents not managed by git), 1),
OPT_BOOL(0, untracked, untracked,
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 try2 02/14] stage: add edit command

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Documentation/git-stage.txt|  5 +++
 builtin/stage.c| 74 ++
 contrib/completion/git-completion.bash |  4 +-
 3 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-stage.txt b/Documentation/git-stage.txt
index 5b42b29..13a01c8 100644
--- a/Documentation/git-stage.txt
+++ b/Documentation/git-stage.txt
@@ -15,6 +15,7 @@ SYNOPSIS
 'git stage diff' [options] [commit] [--] [paths...]
 'git stage rm' [options] [--] [paths...]
 'git stage apply' [options] [--] [paths...]
+'git stage edit'
 
 DESCRIPTION
 ---
@@ -46,6 +47,10 @@ Remove files from the staging area only. See 
linkgit:git-rm[1] --staged.
 
 Apply a patch to the staging area. See linkgit:git-rm[1] --staged.
 
+'edit'::
+
+Manually edit the staging area (as a diff).
+
 SEE ALSO
 
 linkgit:git-add[1]
diff --git a/builtin/stage.c b/builtin/stage.c
index 7c4d442..f537c1d 100644
--- a/builtin/stage.c
+++ b/builtin/stage.c
@@ -6,6 +6,9 @@
 
 #include builtin.h
 #include parse-options.h
+#include diff.h
+#include diffcore.h
+#include revision.h
 
 static const char *const stage_usage[] = {
N_(git stage [options] [--] paths...),
@@ -17,6 +20,74 @@ static const char *const stage_usage[] = {
NULL
 };
 
+static int do_reset(const char *prefix)
+{
+   const char *argv[] = { reset, --quiet, NULL };
+   return cmd_reset(2, argv, prefix);
+}
+
+static int do_apply(const char *file, const char *prefix)
+{
+   const char *argv[] = { apply, --recount, --cached, file, NULL };
+   return cmd_apply(4, argv, prefix);
+}
+
+static int edit(int argc, const char **argv, const char *prefix)
+{
+   char *file = git_pathdup(STAGE_EDIT.patch);
+   int out;
+   struct rev_info rev;
+   int ret = 0;
+   struct stat st;
+
+   read_cache();
+
+   init_revisions(rev, prefix);
+   rev.diffopt.context = 7;
+
+   argc = setup_revisions(argc, argv, rev, NULL);
+   add_head_to_pending(rev);
+   if (!rev.pending.nr) {
+   struct tree *tree;
+   tree = lookup_tree(EMPTY_TREE_SHA1_BIN);
+   add_pending_object(rev, tree-object, HEAD);
+   }
+
+   rev.diffopt.output_format = DIFF_FORMAT_PATCH;
+   rev.diffopt.use_color = 0;
+   DIFF_OPT_SET(rev.diffopt, IGNORE_DIRTY_SUBMODULES);
+
+   out = open(file, O_CREAT | O_WRONLY, 0666);
+   if (out  0)
+   die(_(Could not open '%s' for writing.), file);
+   rev.diffopt.file = xfdopen(out, w);
+   rev.diffopt.close_file = 1;
+
+   if (run_diff_index(rev, 1))
+   die(_(Could not write patch));
+   if (launch_editor(file, NULL, NULL))
+   exit(1);
+
+   if (stat(file, st))
+   die_errno(_(Could not stat '%s'), file);
+
+   ret = do_reset(prefix);
+   if (ret)
+   goto leave;
+
+   if (!st.st_size)
+   goto leave;
+
+   ret = do_apply(file, prefix);
+   if (ret)
+   goto leave;
+
+leave:
+   unlink(file);
+   free(file);
+   return ret;
+}
+
 int cmd_stage(int argc, const char **argv, const char *prefix)
 {
struct option options[] = { OPT_END() };
@@ -47,6 +118,9 @@ int cmd_stage(int argc, const char **argv, const char 
*prefix)
 
return cmd_apply(argc, argv, prefix);
}
+   if (!strcmp(argv[1], edit)) {
+   return edit(argc - 1, argv + 1, prefix);
+   }
}
 
return cmd_add(argc, argv, prefix);
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 0521d52..2ec7b1a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1707,7 +1707,7 @@ _git_stage ()
 {
__git_has_doubledash  return
 
-   local subcommands=add reset diff rm apply
+   local subcommands=add reset diff rm apply edit
local subcommand=$(__git_find_on_cmdline $subcommands)
if [ -z $subcommand ]; then
__gitcomp $subcommands
@@ -1725,6 +1725,8 @@ _git_stage ()
_git_rm;;
apply)
_git_apply;;
+   edit)
+   ;;
*)
_git_add;
esac
-- 
1.9.2+fc1.2.gfbaae8c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

On Fri, Apr 25, 2014 at 12:45:27PM -0500, Felipe Contreras wrote:

 When I say literally everbody agreed to move away from the name index 
 (except
 Junio and another guy) I mean it. I even composed a list:
 
 http://article.gmane.org/gmane.comp.version-control.git/233469
 
 Jeff King, Jonathan Nieder, Matthieu Moy, they all agreed.

With reference to my name, your email says:

  Jeff King:
  staging area makes perfect sense

But here's that statement in context[1]:

  So the term staging area makes perfect sense to me; it is where we
  collect changes to make a commit. I am willing to accept that does not
  to others (native English speakers or no), and that we may need to
  come up with a better term. But I think just calling it the stage is
  even worse; it loses the concept that it is a place for collecting and
  organizing.

In other words, I was saying that the term makes sense to me, and
primarily comparing favorably to a worse proposal. I am not commenting
at all on a plan to change names, which is what you are claiming above.

I do think the term staging area is fine, but picking a term is only
step one of a plan. The rest is deciding how to make the change, and
whether it is worth it. I remain undecided on the latter bits. Please
don't quote me out of context in a way that implies that I am decided.

-Peff

[1] http://article.gmane.org/gmane.comp.version-control.git/168012
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert Stop starting pager recursively

2014-04-25 Thread Jörn Engel

On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote:
 
 This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84.
 
 Caused a usability regression for me and foul language for my coworkers.

Ping.

Jörn

--
People really ought to be forced to read their code aloud over the phone.
That would rapidly improve the choice of identifiers.
-- Al Viro
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[GSoC 2014] Accepted GSoC student introduction

2014-04-25 Thread tanay abhra

Hi,

I am one of the three selected GSoC(Google summer of Code) students by
Git this year.
I am a third year computer science student from India. First, I want to thank
you all for guiding me through the patch review process and being
patient with me. I learned a lot from you all
and this helped me polishing my proposal, so I want to thank you all
for giving me this opportunity.

My mentors for the project are Matthieu Moy and Ramkumar Ramachandra. My
proposal topic is Git configuration API improvements and I am
appending the whole proposal below
ad verbatim for you all to review. I had posted it earlier but it may
have gotten lost in the noise.
I am looking forward to having a great summer working on git.


##GSoC Proposal : Git configuration API improvements


Abstract:
---

The current method for reading values from the config file (summarized
from reading config.c, bulitin/config.c and
api-config.txt)in the disk is parsing and reading the file each time a
function calls for a config value.
git_config() feeds every variable and value to the callback function
which decides what to do with them. Due to
this approach, it is not uncommon for the configuration to be parsed
several times during the run of a Git
program, with different callbacks picking out different variables
useful to themselves. I propose that instead of
reading and parsing the config file multiple times from the disk
during a process run, we will construct a
in-memory representation of the config file the first time a config
value is queried for. Also, the callbacks will
be rewired to use the new API functions provided for efficient lookups
from the internal cache. After this
implementation is validated, new features can be added such as
unsetting values from config file.



#Proposed Improvements

* Fix git config --unset to clean up detritus from sections that are
left empty and some more aesthetic
concerns illustrated below .

* Read the configuration from files once and cache the results in an
appropriate data structure in memory.

* Change git_config() to iterate through the pre-read values in memory
rather than re-reading the configuration
  files.

* Add new API calls that allow the cache to be inquired easily and efficiently.

* Rewrite callers to use the new API wherever possible.

#Possible Problems with this approach,

* How to invalidate the cache correctly in the case that the
configuration is changed while `git` is executing.

* What to do with the comments, which are generally found in config
files, and can be places anywhere(above a
  section header , below a section header etc.)?

#Future Improvements

* Allow configuration values to be unset via a config file

--
##Changing the git_config api to retrieve values from memory
--

#Current Implementation of the git-config subsystem:


Config files are parsed linearly, and each variable found is passed to
a caller-provided callback function. The
callback function is responsible for any actions to be taken on the
config option, and is free to ignore
some options. It is not uncommon for the configuration to be parsed
several times during the run of a Git program,
with different callbacks picking out different variables useful to themselves.

A config callback function takes three parameters:

- the name of the parsed variable. This is in canonical flat form: the
  section, subsection, and variable segments will be separated by dots,
  and the section and variable segments will be all lowercase. E.g.,
  `core.ignorecase`, `diff.SomeType.textconv`.

- the value of the found variable, as a string. If the variable had no
  value specified, the value will be NULL (typically this means it
  should be interpreted as boolean true).

- a void pointer passed in by the caller of the config API; this can
  contain callback-specific data

A config callback should return 0 for success, or -1 if the variable
could not be parsed properly.

#Current Config Querying:
---

Most code in git,calls `git_config` to look up variables in all config
files that Git knows about, using the normal precedence rules.
To do this, it calls with a callback function and void data pointer.

`git_config` will read all config sources in order of increasing
priority. The logic flow is attached as a image[1]
. Thus a callback should typically overwrite previously-seenentries
with new ones (e.g., if both the user-wide
`~/.gitconfig` and repo-specific `.git/config` contains `color.ui`,
the config machinery will first feed the
user-wide one to the callback, and then the repo-specific one; by
overwriting, the higher-priority repo-specific
value is left at the end).

The current precedence order rates the repo-config file the highest.

The `git_config_with_options` function lets the caller examine

Re: What is missing from Git v2.0

Jeff King wrote:
 On Fri, Apr 25, 2014 at 12:45:27PM -0500, Felipe Contreras wrote:
 
  When I say literally everbody agreed to move away from the name index 
  (except
  Junio and another guy) I mean it. I even composed a list:
  
  http://article.gmane.org/gmane.comp.version-control.git/233469
  
  Jeff King, Jonathan Nieder, Matthieu Moy, they all agreed.
 
 With reference to my name, your email says:
 
   Jeff King:
   staging area makes perfect sense
 
 But here's that statement in context[1]:
 
   So the term staging area makes perfect sense to me; it is where we
   collect changes to make a commit. I am willing to accept that does not
   to others (native English speakers or no), and that we may need to
   come up with a better term. But I think just calling it the stage is
   even worse; it loses the concept that it is a place for collecting and
   organizing.
 
 In other words, I was saying that the term makes sense to me, and
 primarily comparing favorably to a worse proposal. I am not commenting
 at all on a plan to change names, which is what you are claiming above.
 
 I do think the term staging area is fine, but picking a term is only
 step one of a plan. The rest is deciding how to make the change, and
 whether it is worth it. I remain undecided on the latter bits. Please
 don't quote me out of context in a way that implies that I am decided.

I specifically said everybody agreed to move away from the name 'index', I
didn't say everybody agreed on the staged area although the vast majority
did, and I didn't say anybody agreed on my patches, although some did.

I think I was clear.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert Stop starting pager recursively

2014-04-25 Thread Matthieu Moy

Jörn Engel jo...@logfs.org writes:

 On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote:
 
 This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84.
 
 Caused a usability regression for me and foul language for my coworkers.

 Ping.

How do you solve the problem that the commit you revert was solving? The
commit you propose to revert says in its message:

git-column can be used as a pager for other git commands, something
like this:

GIT_PAGER=git -p column --mode='dense color' git -p branch

The problem with this is that git -p column also has $GIT_PAGER set so
the pager runs itself again as another pager. The end result is an
infinite loop of forking.

There's probably a solution, but you can't ignore the problem (or
someone else will later try to solve the infinite loop and revert your
commit, and so on ...).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

On Fri, Apr 25, 2014 at 01:27:17PM -0500, Felipe Contreras wrote:

 I specifically said everybody agreed to move away from the name 'index', I
 didn't say everybody agreed on the staged area although the vast majority
 did, and I didn't say anybody agreed on my patches, although some did.
 
 I think I was clear.

Maybe I was not clear in my response, so let me try again. I do _not_
necessarily agree that we need to move away from the name index. I am
specifically saying that your everyone includes my name explicitly,
and that I do not agree with the statement you are attributing to it.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v11 02/11] trailer: process trailers from input message and arguments

Implement the logic to process trailers from the input message
and from arguments.

At the beginning trailers from the input message are in their
own in_tok doubly linked list, and trailers from arguments
are in their own arg_tok doubly linked list.

The lists are traversed and when an arg_tok should be applied,
it is removed from its list and inserted into the in_tok list.

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 trailer.c | 198 ++
 1 file changed, 198 insertions(+)

diff --git a/trailer.c b/trailer.c
index db93a63..52108c2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -47,3 +47,201 @@ static size_t alnum_len(const char *buf, size_t len)
len--;
return len;
 }
+
+static void free_trailer_item(struct trailer_item *item)
+{
+   free(item-conf.name);
+   free(item-conf.key);
+   free(item-conf.command);
+   free((char *)item-token);
+   free((char *)item-value);
+   free(item);
+}
+
+static void add_arg_to_input_list(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok)
+{
+   if (arg_tok-conf.where == WHERE_AFTER) {
+   arg_tok-next = in_tok-next;
+   in_tok-next = arg_tok;
+   arg_tok-previous = in_tok;
+   if (arg_tok-next)
+   arg_tok-next-previous = arg_tok;
+   } else {
+   arg_tok-previous = in_tok-previous;
+   in_tok-previous = arg_tok;
+   arg_tok-next = in_tok;
+   if (arg_tok-previous)
+   arg_tok-previous-next = arg_tok;
+   }
+}
+
+static int check_if_different(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok,
+ int alnum_len, int check_all)
+{
+   enum action_where where = arg_tok-conf.where;
+   do {
+   if (!in_tok)
+   return 1;
+   if (same_trailer(in_tok, arg_tok, alnum_len))
+   return 0;
+   /*
+* if we want to add a trailer after another one,
+* we have to check those before this one
+*/
+   in_tok = (where == WHERE_AFTER) ? in_tok-previous : 
in_tok-next;
+   } while (check_all);
+   return 1;
+}
+
+static void apply_arg_if_exists(struct trailer_item *in_tok,
+   struct trailer_item *arg_tok,
+   int alnum_len)
+{
+   switch (arg_tok-conf.if_exists) {
+   case EXISTS_DO_NOTHING:
+   free_trailer_item(arg_tok);
+   break;
+   case EXISTS_OVERWRITE:
+   free((char *)in_tok-value);
+   in_tok-value = xstrdup(arg_tok-value);
+   free_trailer_item(arg_tok);
+   break;
+   case EXISTS_ADD:
+   add_arg_to_input_list(in_tok, arg_tok);
+   break;
+   case EXISTS_ADD_IF_DIFFERENT:
+   if (check_if_different(in_tok, arg_tok, alnum_len, 1))
+   add_arg_to_input_list(in_tok, arg_tok);
+   else
+   free_trailer_item(arg_tok);
+   break;
+   case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
+   if (check_if_different(in_tok, arg_tok, alnum_len, 0))
+   add_arg_to_input_list(in_tok, arg_tok);
+   else
+   free_trailer_item(arg_tok);
+   break;
+   }
+}
+
+static void remove_from_list(struct trailer_item *item,
+struct trailer_item **first)
+{
+   if (item-next)
+   item-next-previous = item-previous;
+   if (item-previous)
+   item-previous-next = item-next;
+   else
+   *first = item-next;
+}
+
+static struct trailer_item *remove_first(struct trailer_item **first)
+{
+   struct trailer_item *item = *first;
+   *first = item-next;
+   if (item-next) {
+   item-next-previous = NULL;
+   item-next = NULL;
+   }
+   return item;
+}
+
+static void process_input_token(struct trailer_item *in_tok,
+   struct trailer_item **arg_tok_first,
+   enum action_where where)
+{
+   struct trailer_item *arg_tok;
+   struct trailer_item *next_arg;
+
+   int after = where == WHERE_AFTER;
+   int tok_alnum_len = alnum_len(in_tok-token, strlen(in_tok-token));
+
+   for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+   next_arg = arg_tok-next;
+   if (!same_token(in_tok, arg_tok, tok_alnum_len))
+   continue;
+   if (arg_tok-conf.where != where)
+   continue;
+   remove_from_list(arg_tok, arg_tok_first);
+   apply_arg_if_exists(in_tok,

[PATCH v11 04/11] trailer: process command line trailer arguments

Parse the trailer command line arguments and put
the result into an arg_tok doubly linked list.

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 trailer.c | 118 ++
 1 file changed, 118 insertions(+)

diff --git a/trailer.c b/trailer.c
index f376be5..f79a369 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,4 +1,5 @@
 #include cache.h
+#include string-list.h
 /*
  * Copyright (c) 2013, 2014 Christian Couder chrisc...@tuxfamily.org
  */
@@ -391,3 +392,120 @@ static int git_trailer_config(const char *conf_key, const 
char *value, void *cb)
}
return 0;
 }
+
+static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char 
*trailer)
+{
+   size_t len = strcspn(trailer, =:);
+   if (len == 0)
+   return error(_(empty trailer token in trailer '%s'), trailer);
+   if (len  strlen(trailer)) {
+   strbuf_add(tok, trailer, len);
+   strbuf_trim(tok);
+   strbuf_addstr(val, trailer + len + 1);
+   strbuf_trim(val);
+   } else {
+   strbuf_addstr(tok, trailer);
+   strbuf_trim(tok);
+   }
+   return 0;
+}
+
+
+static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
+{
+   *dst = *src;
+   if (src-name)
+   dst-name = xstrdup(src-name);
+   if (src-key)
+   dst-key = xstrdup(src-key);
+   if (src-command)
+   dst-command = xstrdup(src-command);
+}
+
+static const char *token_from_item(struct trailer_item *item)
+{
+   if (item-conf.key)
+   return item-conf.key;
+
+   return item-conf.name;
+}
+
+static struct trailer_item *new_trailer_item(struct trailer_item *conf_item,
+char *tok, char *val)
+{
+   struct trailer_item *new = xcalloc(sizeof(*new), 1);
+   new-value = val;
+
+   if (conf_item) {
+   duplicate_conf(new-conf, conf_item-conf);
+   new-token = xstrdup(token_from_item(conf_item));
+   free(tok);
+   } else
+   new-token = tok;
+
+   return new;
+}
+
+static int token_matches_item(const char *tok, struct trailer_item *item, int 
alnum_len)
+{
+   if (!strncasecmp(tok, item-conf.name, alnum_len))
+   return 1;
+   return item-conf.key ? !strncasecmp(tok, item-conf.key, alnum_len) : 
0;
+}
+
+static struct trailer_item *create_trailer_item(const char *string)
+{
+   struct strbuf tok = STRBUF_INIT;
+   struct strbuf val = STRBUF_INIT;
+   struct trailer_item *item;
+   int tok_alnum_len;
+
+   if (parse_trailer(tok, val, string))
+   return NULL;
+
+   tok_alnum_len = alnum_len(tok.buf, tok.len);
+
+   /* Lookup if the token matches something in the config */
+   for (item = first_conf_item; item; item = item-next) {
+   if (token_matches_item(tok.buf, item, tok_alnum_len)) {
+   strbuf_release(tok);
+   return new_trailer_item(item,
+   NULL,
+   strbuf_detach(val, NULL));
+   }
+   }
+
+   return new_trailer_item(NULL,
+   strbuf_detach(tok, NULL),
+   strbuf_detach(val, NULL));
+}
+
+static void add_trailer_item(struct trailer_item **first,
+struct trailer_item **last,
+struct trailer_item *new)
+{
+   if (!new)
+   return;
+   if (!*last) {
+   *first = new;
+   *last = new;
+   } else {
+   (*last)-next = new;
+   new-previous = *last;
+   *last = new;
+   }
+}
+
+static struct trailer_item *process_command_line_args(struct string_list 
*trailers)
+{
+   struct trailer_item *arg_tok_first = NULL;
+   struct trailer_item *arg_tok_last = NULL;
+   struct string_list_item *tr;
+
+   for_each_string_list_item(tr, trailers) {
+   struct trailer_item *new = create_trailer_item(tr-string);
+   add_trailer_item(arg_tok_first, arg_tok_last, new);
+   }
+
+   return arg_tok_first;
+}
-- 
1.9.1.636.g20d5f34


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v11 03/11] trailer: read and process config information

Read the configuration to get trailer information, and then process
it and store it in a doubly linked list.

The config information is stored in the list whose first item is
pointed to by:

static struct trailer_item *first_conf_item;

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 trailer.c | 146 ++
 1 file changed, 146 insertions(+)

diff --git a/trailer.c b/trailer.c
index 52108c2..f376be5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -25,6 +25,8 @@ struct trailer_item {
struct conf_info conf;
 };
 
+static struct trailer_item *first_conf_item;
+
 static int same_token(struct trailer_item *a, struct trailer_item *b, int 
alnum_len)
 {
return !strncasecmp(a-token, b-token, alnum_len);
@@ -245,3 +247,147 @@ static void process_trailers_lists(struct trailer_item 
**in_tok_first,
apply_arg_if_missing(in_tok_first, in_tok_last, arg_tok);
}
 }
+
+static int set_where(struct conf_info *item, const char *value)
+{
+   if (!strcasecmp(after, value))
+   item-where = WHERE_AFTER;
+   else if (!strcasecmp(before, value))
+   item-where = WHERE_BEFORE;
+   else
+   return -1;
+   return 0;
+}
+
+static int set_if_exists(struct conf_info *item, const char *value)
+{
+   if (!strcasecmp(addIfDifferent, value))
+   item-if_exists = EXISTS_ADD_IF_DIFFERENT;
+   else if (!strcasecmp(addIfDifferentNeighbor, value))
+   item-if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+   else if (!strcasecmp(add, value))
+   item-if_exists = EXISTS_ADD;
+   else if (!strcasecmp(overwrite, value))
+   item-if_exists = EXISTS_OVERWRITE;
+   else if (!strcasecmp(doNothing, value))
+   item-if_exists = EXISTS_DO_NOTHING;
+   else
+   return -1;
+   return 0;
+}
+
+static int set_if_missing(struct conf_info *item, const char *value)
+{
+   if (!strcasecmp(doNothing, value))
+   item-if_missing = MISSING_DO_NOTHING;
+   else if (!strcasecmp(add, value))
+   item-if_missing = MISSING_ADD;
+   else
+   return -1;
+   return 0;
+}
+
+static struct trailer_item *get_conf_item(const char *name)
+{
+   struct trailer_item *item;
+   struct trailer_item *previous;
+
+   /* Look up item with same name */
+   for (previous = NULL, item = first_conf_item;
+item;
+previous = item, item = item-next) {
+   if (!strcasecmp(item-conf.name, name))
+   return item;
+   }
+
+   /* Item does not already exists, create it */
+   item = xcalloc(sizeof(struct trailer_item), 1);
+   item-conf.name = xstrdup(name);
+
+   if (!previous)
+   first_conf_item = item;
+   else {
+   previous-next = item;
+   item-previous = previous;
+   }
+
+   return item;
+}
+
+enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,
+TRAILER_IF_EXISTS, TRAILER_IF_MISSING };
+
+static struct {
+   const char *name;
+   enum trailer_info_type type;
+} trailer_config_items[] = {
+   { key, TRAILER_KEY },
+   { command, TRAILER_COMMAND },
+   { where, TRAILER_WHERE },
+   { ifexists, TRAILER_IF_EXISTS },
+   { ifmissing, TRAILER_IF_MISSING }
+};
+
+static int git_trailer_config(const char *conf_key, const char *value, void 
*cb)
+{
+   const char *trailer_item, *variable_name;
+   struct trailer_item *item;
+   struct conf_info *conf;
+   char *name = NULL;
+   enum trailer_info_type type;
+   int i;
+
+   trailer_item = skip_prefix(conf_key, trailer.);
+   if (!trailer_item)
+   return 0;
+
+   variable_name = strrchr(trailer_item, '.');
+   if (!variable_name) {
+   warning(_(two level trailer config variable %s), conf_key);
+   return 0;
+   }
+
+   variable_name++;
+   for (i = 0; i  ARRAY_SIZE(trailer_config_items); i++) {
+   if (strcmp(trailer_config_items[i].name, variable_name))
+   continue;
+   name = xstrndup(trailer_item,  variable_name - trailer_item - 
1);
+   type = trailer_config_items[i].type;
+   break;
+   }
+
+   if (!name)
+   return 0;
+
+   item = get_conf_item(name);
+   conf = item-conf;
+   free(name);
+
+   switch (type) {
+   case TRAILER_KEY:
+   if (conf-key)
+   warning(_(more than one %s), conf_key);
+   conf-key = xstrdup(value);
+   break;
+   case TRAILER_COMMAND:
+   if (conf-command)
+   warning(_(more than one %s), conf_key);
+   conf-command = xstrdup(value);
+   break;
+   case

Re: What is missing from Git v2.0

Jeff King wrote:
 On Fri, Apr 25, 2014 at 01:27:17PM -0500, Felipe Contreras wrote:
 
  I specifically said everybody agreed to move away from the name 'index', I
  didn't say everybody agreed on the staged area although the vast majority
  did, and I didn't say anybody agreed on my patches, although some did.
  
  I think I was clear.
 
 Maybe I was not clear in my response, so let me try again. I do _not_
 necessarily agree that we need to move away from the name index.

So you agree that the index is a bad name, and you agree staging area is a
better name, yet you don't agree we should move away from the term index?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v11 05/11] trailer: parse trailers from file or stdin

Read trailers from a file or from stdin, parse the trailers and then
put the result into a doubly linked list.

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 trailer.c | 116 ++
 1 file changed, 116 insertions(+)

diff --git a/trailer.c b/trailer.c
index f79a369..4ca9157 100644
--- a/trailer.c
+++ b/trailer.c
@@ -51,6 +51,14 @@ static size_t alnum_len(const char *buf, size_t len)
return len;
 }
 
+static inline int contains_only_spaces(const char *str)
+{
+   const char *s = str;
+   while (*s  isspace(*s))
+   s++;
+   return !*s;
+}
+
 static void free_trailer_item(struct trailer_item *item)
 {
free(item-conf.name);
@@ -509,3 +517,111 @@ static struct trailer_item 
*process_command_line_args(struct string_list *traile
 
return arg_tok_first;
 }
+
+static struct strbuf **read_input_file(const char *file)
+{
+   struct strbuf **lines;
+   struct strbuf sb = STRBUF_INIT;
+
+   if (file) {
+   if (strbuf_read_file(sb, file, 0)  0)
+   die_errno(_(could not read input file '%s'), file);
+   } else {
+   if (strbuf_read(sb, fileno(stdin), 0)  0)
+   die_errno(_(could not read from stdin));
+   }
+
+   lines = strbuf_split(sb, '\n');
+
+   strbuf_release(sb);
+
+   return lines;
+}
+
+/*
+ * Return the (0 based) index of the start of the patch or the line
+ * count if there is no patch in the message.
+ */
+static int find_patch_start(struct strbuf **lines, int count)
+{
+   int i;
+
+   /* Get the start of the patch part if any */
+   for (i = 0; i  count; i++) {
+   if (starts_with(lines[i]-buf, ---))
+   return i;
+   }
+
+   return count;
+}
+
+/*
+ * Return the (0 based) index of the first trailer line or count if
+ * there are no trailers. Trailers are searched only in the lines from
+ * index (count - 1) down to index 0. The has_blank_line parameter
+ * tells if there is a blank line before the trailers.
+ */
+static int find_trailer_start(struct strbuf **lines, int count, int 
*has_blank_line)
+{
+   int start, only_spaces = 1;
+
+   /*
+* Get the start of the trailers by looking starting from the end
+* for a line with only spaces before lines with one ':'.
+*/
+   for (start = count - 1; start = 0; start--) {
+   if (contains_only_spaces(lines[start]-buf)) {
+   if (only_spaces)
+   continue;
+   *has_blank_line = 1;
+   return start + 1;
+   }
+   if (strchr(lines[start]-buf, ':')) {
+   if (only_spaces)
+   only_spaces = 0;
+   continue;
+   }
+   *has_blank_line = start == count - 1 ?
+ 0 : contains_only_spaces(lines[start + 1]-buf);
+   return count;
+   }
+
+   *has_blank_line = only_spaces ? count  0 : 0;
+   return only_spaces ? count : start + 1;
+}
+
+static void print_lines(struct strbuf **lines, int start, int end)
+{
+   int i;
+   for (i = start; lines[i]  i  end; i++)
+   printf(%s, lines[i]-buf);
+}
+
+static int process_input_file(struct strbuf **lines,
+ struct trailer_item **in_tok_first,
+ struct trailer_item **in_tok_last)
+{
+   int count = 0;
+   int patch_start, trailer_start, has_blank_line, i;
+
+   /* Get the line count */
+   while (lines[count])
+   count++;
+
+   patch_start = find_patch_start(lines, count);
+   trailer_start = find_trailer_start(lines, patch_start, has_blank_line);
+
+   /* Print lines before the trailers as is */
+   print_lines(lines, 0, trailer_start);
+
+   if (!has_blank_line)
+   printf(\n);
+
+   /* Parse trailer lines */
+   for (i = trailer_start; i  patch_start; i++) {
+   struct trailer_item *new = create_trailer_item(lines[i]-buf);
+   add_trailer_item(in_tok_first, in_tok_last, new);
+   }
+
+   return patch_start;
+}
-- 
1.9.1.636.g20d5f34


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v11 11/11] Documentation: add documentation for 'git interpret-trailers'

While at it add git-interpret-trailers to command-list.txt.

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 Documentation/git-interpret-trailers.txt | 143 +++
 command-list.txt |   1 +
 2 files changed, 144 insertions(+)
 create mode 100644 Documentation/git-interpret-trailers.txt

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
new file mode 100644
index 000..450ec54
--- /dev/null
+++ b/Documentation/git-interpret-trailers.txt
@@ -0,0 +1,143 @@
+git-interpret-trailers(1)
+=
+
+NAME
+
+git-interpret-trailers - help add stuctured information into commit messages
+
+SYNOPSIS
+
+[verse]
+'git interpret-trailers' [--trim-empty] [(--trailer token[(=|:)value])...] 
[file...]
+
+DESCRIPTION
+---
+Help adding 'trailers' lines, that look similar to RFC 822 e-mail
+headers, at the end of the otherwise free-form part of a commit
+message.
+
+This command reads some patches or commit messages from either the
+file arguments or the standard input if no file is specified. Then
+this command applies the arguments passed using the `--trailer`
+option, if any, to the commit message part of each input file. The
+result is emitted on the standard output.
+
+Some configuration variables control the way the `--trailer` arguments
+are applied to each commit message and the way any existing trailer in
+the commit message is changed. They also make it possible to
+automatically add some trailers.
+
+By default, a 'token=value' or 'token:value' argument given
+using `--trailer` will be added only if no trailer with the same
+(token, value) pair is already in the message. The token and
+value parts will be trimmed to remove starting and trailing
+whitespace, and the resulting trimmed token and value will appear
+in the message like this:
+
+
+token: value
+
+
+By default, if there are already trailers with the same token, the
+new trailer will appear just after the last trailer with the same
+token. Otherwise it will appear at the end of the message.
+
+The trailers are recognized in the input commit message using the
+following rules:
+
+* only lines that contains a ':' are considered trailers,
+* the trailer lines must all be next to each other,
+* after them it's only possible to have some lines that contain only spaces,
+* before them there must be at least one line with only spaces.
+
+Note that 'trailers' do not follow and are not intended to follow many
+rules for RFC 822 headers. For example they do not follow the line
+folding rules, the encoding rules and probably many other rules.
+
+OPTIONS
+---
+--trim-empty::
+   If the value part of any trailer contains only whitespace,
+   the whole trailer will be removed from the resulting message.
+   This apply to existing trailers as well as new trailers.
+
+--trailer token[(=|:)value]::
+   Specify a (token, value) pair that should be applied as a
+   trailer to the input messages. See the description of this
+   command.
+
+CONFIGURATION VARIABLES
+---
+
+trailer.token.key::
+   This `key` will be used instead of token in the
+   trailer. After the last alphanumeric character, it can contain
+   some non alphanumeric characters like ':', '=' or '#' that
+   will be used instead of ':' to separate the token from the
+   value in the trailer, though the default ':' is more
+   standard.
+
+trailer.token.where::
+   This can be either `after`, which is the default, or
+   `before`. If it is `before`, then a trailer with the specified
+   token, will appear before, instead of after, other trailers
+   with the same token, or otherwise at the beginning, instead
+   of at the end, of all the trailers.
+
+trailer.token.ifexist::
+   This option makes it possible to choose what action will be
+   performed when there is already at least one trailer with the
+   same token in the message.
++
+The valid values for this option are: `addIfDifferent` (this is the
+default), `addIfDifferentNeighbor`, `add`, `overwrite` or `doNothing`.
++
+With `addIfDifferent`, a new trailer will be added only if no trailer
+with the same (token, value) pair is already in the message.
++
+With `addIfDifferentNeighbor`, a new trailer will be added only if no
+trailer with the same (token, value) pair is above or below the line
+where the new trailer will be added.
++
+With `add`, a new trailer will be added, even if some trailers with
+the same (token, value) pair are already in the message.
++
+With `overwrite`, the new trailer will overwrite an existing trailer
+with the same token.
++
+With `doNothing`, nothing will be done; that is no new trailer will be
+added if there is already one with the same

[PATCH v11 00/11] Add interpret-trailers builtin

This patch series implements a new command:

git interpret-trailers

and an infrastructure to process trailers that can be reused,
for example in commit.c.

1) Rationale:

This command should help with RFC 822 style headers, called
trailers, that are found at the end of commit messages.

(Note that these headers do not follow and are not intended to
follow many rules that are in RFC 822. For example they do not
follow the line breaking rules, the encoding rules and probably
many other rules.)

For a long time, these trailers have become a de facto standard
way to add helpful information into commit messages.

Until now git commit has only supported the well known
Signed-off-by:  trailer, that is used by many projects like
the Linux kernel and Git.

It is better to keep builtin/commit.c uncontaminated by any more
hard-wired logic, like what we have for the signed-off-by line.  Any
new things can and should be doable in hooks, and this filter would
help writing these hooks.

And that is why the design goal of the filter is to make it at least
as powerful as the built-in logic we have for signed-off-by lines;
that would allow us to later eject the hard-wired logic for
signed-off-by line from the main codepath, if/when we wanted to.

Alternatively, we could build a library-ish API around this filter
code and replace the hard-wired logic for signed-off-by line with a
call into that API, if/when we wanted to, but that requires (in
addition to the at least as powerful as the built-in logic) that
the implementation of this stand-alone filter can be cleanly made
into a reusable library, so that is a bit higher bar to cross than
everything can be doable with hooks alternative.

2) Current state:

Currently the usage string of this command is:

git interpret-trailers [--trim-empty] [(--trailer token[(=|:)value])...] 
[file...]

The following features are implemented:

- the result is printed on stdout
- the --trailer arguments are interpreted
- messages read from file... (new) or stdin are interpreted
- the trailer.token.key options in the config are interpreted
- the trailer.token.where options are interpreted
- the trailer.token.ifExist options are interpreted
- the trailer.token.ifMissing options are interpreted
- the trailer.token.command config works
- $ARG can be used in commands
- messages can contain a patch (new)
- there are 34 tests
- there is some documentation

The following features are planned but not yet implemented:
- add examples in documentation

Possible improvements:
- integration with git commit
- support GIT_COMMIT_PROTO env variable in commands

3) Changes since version 10, thanks to Michael and Junio:

* changed the usage string and the interface
* detect a patch in the input and print it if any
* adapt tests and add some related to the above changes
* improved documentation
* squashed patch to add blank lines before trailers into
  other pathes
* added git-interpret-trailers to command-list.txt
* use strcasecmp() instead of strcmp() to compare
  some values from the config file matching a fixed set

This means that only patches 1/11, 2/11, 3/11, 9/11 and 10/11
mostly did not change. I say mostly because there are still
at least a commit message change in 2/11 and
s/strcmp/strcasecmp/ in 3/11.

Christian Couder (11):
  trailer: add data structures and basic functions
  trailer: process trailers from input message and arguments
  trailer: read and process config information
  trailer: process command line trailer arguments
  trailer: parse trailers from file or stdin
  trailer: put all the processing together and print
  trailer: add interpret-trailers command
  trailer: add tests for git interpret-trailers
  trailer: execute command from 'trailer.name.command'
  trailer: add tests for commands in config file
  Documentation: add documentation for 'git interpret-trailers'

 .gitignore   |   1 +
 Documentation/git-interpret-trailers.txt | 143 ++
 Makefile |   2 +
 builtin.h|   1 +
 builtin/interpret-trailers.c |  44 ++
 command-list.txt |   1 +
 git.c|   1 +
 t/t7513-interpret-trailers.sh| 542 ++
 trailer.c| 750 +++
 trailer.h|   6 +
 10 files changed, 1491 insertions(+)
 create mode 100644 Documentation/git-interpret-trailers.txt
 create mode 100644 builtin/interpret-trailers.c
 create mode 100755 t/t7513-interpret-trailers.sh
 create mode 100644 trailer.c
 create mode 100644 trailer.h

-- 
1.9.1.636.g20d5f34

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at

Re: What is missing from Git v2.0

On Fri, Apr 25, 2014 at 01:57:59PM -0500, Felipe Contreras wrote:

  Maybe I was not clear in my response, so let me try again. I do _not_
  necessarily agree that we need to move away from the name index.
 
 So you agree that the index is a bad name, and you agree staging area is a
 better name, yet you don't agree we should move away from the term index?

I don't agree that the index is a bad name, because that implies
some objective level of bad.

I do think the name staging area is fine, and I think it may even be
better than index, if we were picking a name out of the blue.

I am undecided on whether we should move away from the term index. The
way you have phrased it seems like you are trying to create a logical
contradiction: A is bad, B is good, therefore we should move from A to
B. But that neglects the cost of moving.

Frankly, I am not that interested in discussing it with you. I _am_
interested in you not using my name to claim that I believe things I do
not.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v11 08/11] trailer: add tests for git interpret-trailers

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 t/t7513-interpret-trailers.sh | 418 ++
 1 file changed, 418 insertions(+)
 create mode 100755 t/t7513-interpret-trailers.sh

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
new file mode 100755
index 000..4506e18
--- /dev/null
+++ b/t/t7513-interpret-trailers.sh
@@ -0,0 +1,418 @@
+#!/bin/sh
+#
+# Copyright (c) 2013, 2014 Christian Couder
+#
+
+test_description='git interpret-trailers'
+
+. ./test-lib.sh
+
+# When we want one trailing space at the end of each line, let's use sed
+# to make sure that these spaces are not removed by any automatic tool.
+
+test_expect_success 'setup' '
+   cat basic_message -\EOF 
+   subject
+
+   body
+   EOF
+   cat complex_message_body -\EOF 
+   my subject
+
+   my body which is long
+   and contains some special
+   chars like : = ? !
+
+   EOF
+   sed -e s/ Z\$/ / complex_message_trailers -\EOF 
+   Fixes: Z
+   Acked-by: Z
+   Reviewed-by: Z
+   Signed-off-by: Z
+   EOF
+   cat basic_patch -\EOF
+   ---
+foo.txt | 2 +-
+1 file changed, 1 insertion(+), 1 deletion(-)
+
+   diff --git a/foo.txt b/foo.txt
+   index 0353767..1d91aa1 100644
+   --- a/foo.txt
+   +++ b/foo.txt
+   @@ -1,3 +1,3 @@
+
+   -bar
+   +baz
+
+   --
+   1.9.rc0.11.ga562ddc
+
+   EOF
+'
+
+test_expect_success 'without config' '
+   sed -e s/ Z\$/ / expected -\EOF 
+
+   ack: Peff
+   Reviewed-by: Z
+   Acked-by: Johan
+   EOF
+   git interpret-trailers --trailer ack = Peff --trailer Reviewed-by \
+   --trailer Acked-by: Johan actual 
+   test_cmp expected actual
+'
+
+test_expect_success '--trim-empty without config' '
+   cat expected -\EOF 
+
+   ack: Peff
+   Acked-by: Johan
+   EOF
+   git interpret-trailers --trim-empty --trailer ack = Peff \
+   --trailer Reviewed-by --trailer Acked-by: Johan \
+   --trailer sob: actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'with config setup' '
+   git config trailer.ack.key Acked-by:  
+   cat expected -\EOF 
+
+   Acked-by: Peff
+   EOF
+   git interpret-trailers --trim-empty --trailer ack = Peff actual 
+   test_cmp expected actual 
+   git interpret-trailers --trim-empty --trailer Acked-by = Peff actual 

+   test_cmp expected actual 
+   git interpret-trailers --trim-empty --trailer Acked-by :Peff actual 

+   test_cmp expected actual
+'
+
+test_expect_success 'with config setup and = sign' '
+   git config trailer.ack.key Acked-by=  
+   cat expected -\EOF 
+
+   Acked-by= Peff
+   EOF
+   git interpret-trailers --trim-empty --trailer ack = Peff actual 
+   test_cmp expected actual 
+   git interpret-trailers --trim-empty --trailer Acked-by= Peff actual 

+   test_cmp expected actual 
+   git interpret-trailers --trim-empty --trailer Acked-by : Peff actual 

+   test_cmp expected actual
+'
+
+test_expect_success 'with config setup and # sign' '
+   git config trailer.bug.key Bug # 
+   cat expected -\EOF 
+
+   Bug #42
+   EOF
+   git interpret-trailers --trim-empty --trailer bug = 42 actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'with commit basic message' '
+   cat basic_message expected 
+   echo expected 
+   git interpret-trailers basic_message actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'with basic patch' '
+   cat basic_message input 
+   cat basic_patch input 
+   cat basic_message expected 
+   echo expected 
+   cat basic_patch expected 
+   git interpret-trailers input actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'with commit complex message as argument' '
+   cat complex_message_body complex_message_trailers complex_message 
+   cat complex_message_body expected 
+   sed -e s/ Z\$/ / expected -\EOF 
+   Fixes: Z
+   Acked-by= Z
+   Reviewed-by: Z
+   Signed-off-by: Z
+   EOF
+   git interpret-trailers complex_message actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'with 2 files arguments' '
+   cat basic_message expected 
+   echo expected 
+   cat basic_patch expected 
+   git interpret-trailers complex_message input actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'with commit complex message and trailer args' '
+   cat complex_message_body expected 
+   sed -e s/ Z\$/ /

[PATCH v11 07/11] trailer: add interpret-trailers command

This patch adds the git interpret-trailers command.
This command uses the previously added process_trailers()
function in trailer.c.

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 .gitignore   |  1 +
 Makefile |  1 +
 builtin.h|  1 +
 builtin/interpret-trailers.c | 44 
 git.c|  1 +
 5 files changed, 48 insertions(+)
 create mode 100644 builtin/interpret-trailers.c

diff --git a/.gitignore b/.gitignore
index b5f9def..c870ada 100644
--- a/.gitignore
+++ b/.gitignore
@@ -74,6 +74,7 @@
 /git-index-pack
 /git-init
 /git-init-db
+/git-interpret-trailers
 /git-instaweb
 /git-log
 /git-ls-files
diff --git a/Makefile b/Makefile
index ec90feb..a91465e 100644
--- a/Makefile
+++ b/Makefile
@@ -935,6 +935,7 @@ BUILTIN_OBJS += builtin/hash-object.o
 BUILTIN_OBJS += builtin/help.o
 BUILTIN_OBJS += builtin/index-pack.o
 BUILTIN_OBJS += builtin/init-db.o
+BUILTIN_OBJS += builtin/interpret-trailers.o
 BUILTIN_OBJS += builtin/log.o
 BUILTIN_OBJS += builtin/ls-files.o
 BUILTIN_OBJS += builtin/ls-remote.o
diff --git a/builtin.h b/builtin.h
index c47c110..8ca0065 100644
--- a/builtin.h
+++ b/builtin.h
@@ -73,6 +73,7 @@ extern int cmd_hash_object(int argc, const char **argv, const 
char *prefix);
 extern int cmd_help(int argc, const char **argv, const char *prefix);
 extern int cmd_index_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_init_db(int argc, const char **argv, const char *prefix);
+extern int cmd_interpret_trailers(int argc, const char **argv, const char 
*prefix);
 extern int cmd_log(int argc, const char **argv, const char *prefix);
 extern int cmd_log_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_ls_files(int argc, const char **argv, const char *prefix);
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
new file mode 100644
index 000..46838d2
--- /dev/null
+++ b/builtin/interpret-trailers.c
@@ -0,0 +1,44 @@
+/*
+ * Builtin git interpret-trailers
+ *
+ * Copyright (c) 2013, 2014 Christian Couder chrisc...@tuxfamily.org
+ *
+ */
+
+#include cache.h
+#include builtin.h
+#include parse-options.h
+#include string-list.h
+#include trailer.h
+
+static const char * const git_interpret_trailers_usage[] = {
+   N_(git interpret-trailers [--trim-empty] [(--trailer 
token[(=|:)value])...] [file...]),
+   NULL
+};
+
+int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
+{
+   int trim_empty = 0;
+   struct string_list trailers = STRING_LIST_INIT_DUP;
+
+   struct option options[] = {
+   OPT_BOOL(0, trim-empty, trim_empty, N_(trim empty 
trailers)),
+   OPT_STRING_LIST(0, trailer, trailers, N_(trailer),
+   N_(trailer(s) to add)),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_interpret_trailers_usage, 0);
+
+   if (argc) {
+   int i;
+   for (i = 0; i  argc; i++)
+   process_trailers(argv[i], trim_empty, trailers);
+   } else
+   process_trailers(NULL, trim_empty, trailers);
+
+   string_list_clear(trailers, 0);
+
+   return 0;
+}
diff --git a/git.c b/git.c
index 7cf2953..63a03eb 100644
--- a/git.c
+++ b/git.c
@@ -380,6 +380,7 @@ static struct cmd_struct commands[] = {
{ index-pack, cmd_index_pack, RUN_SETUP_GENTLY },
{ init, cmd_init_db },
{ init-db, cmd_init_db },
+   { interpret-trailers, cmd_interpret_trailers, RUN_SETUP },
{ log, cmd_log, RUN_SETUP },
{ ls-files, cmd_ls_files, RUN_SETUP },
{ ls-remote, cmd_ls_remote, RUN_SETUP_GENTLY },
-- 
1.9.1.636.g20d5f34


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v11 10/11] trailer: add tests for commands in config file

And add a few other tests for some special cases.

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 t/t7513-interpret-trailers.sh | 124 ++
 1 file changed, 124 insertions(+)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 4506e18..9aae721 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -415,4 +415,128 @@ test_expect_success 'using ifMissing = doNothing' '
test_cmp expected actual
 '
 
+test_expect_success 'with simple command' '
+   git config trailer.sign.key Signed-off-by:  
+   git config trailer.sign.where after 
+   git config trailer.sign.ifExists addIfDifferentNeighbor 
+   git config trailer.sign.command echo \A U Thor 
aut...@example.com\ 
+   cat complex_message_body expected 
+   sed -e s/ Z\$/ / expected -\EOF 
+   Fixes: Z
+   Acked-by= Z
+   Reviewed-by: Z
+   Signed-off-by: Z
+   Signed-off-by: A U Thor aut...@example.com
+   EOF
+   git interpret-trailers --trailer review: --trailer fix=22 \
+   complex_message actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'with command using commiter information' '
+   git config trailer.sign.ifExists addIfDifferent 
+   git config trailer.sign.command echo \\$GIT_COMMITTER_NAME 
\$GIT_COMMITTER_EMAIL\ 
+   cat complex_message_body expected 
+   sed -e s/ Z\$/ / expected -\EOF 
+   Fixes: Z
+   Acked-by= Z
+   Reviewed-by: Z
+   Signed-off-by: Z
+   Signed-off-by: C O Mitter commit...@example.com
+   EOF
+   git interpret-trailers --trailer review: --trailer fix=22 \
+   complex_message actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'with command using author information' '
+   git config trailer.sign.key Signed-off-by:  
+   git config trailer.sign.where after 
+   git config trailer.sign.ifExists addIfDifferentNeighbor 
+   git config trailer.sign.command echo \\$GIT_AUTHOR_NAME 
\$GIT_AUTHOR_EMAIL\ 
+   cat complex_message_body expected 
+   sed -e s/ Z\$/ / expected -\EOF 
+   Fixes: Z
+   Acked-by= Z
+   Reviewed-by: Z
+   Signed-off-by: Z
+   Signed-off-by: A U Thor aut...@example.com
+   EOF
+   git interpret-trailers --trailer review: --trailer fix=22 \
+   complex_message actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'setup a commit' '
+   echo Content of the first commit.  a.txt 
+   git add a.txt 
+   git commit -m Add file a.txt
+'
+
+test_expect_success 'with command using $ARG' '
+   git config trailer.fix.ifExists overwrite 
+   git config trailer.fix.command git log -1 --oneline --format=\%h 
(%s)\ --abbrev-commit --abbrev=14 \$ARG 
+   FIXED=$(git log -1 --oneline --format=%h (%s) --abbrev-commit 
--abbrev=14 HEAD) 
+   cat complex_message_body expected 
+   sed -e s/ Z\$/ / expected -EOF 
+   Fixes: $FIXED
+   Acked-by= Z
+   Reviewed-by: Z
+   Signed-off-by: Z
+   Signed-off-by: A U Thor aut...@example.com
+   EOF
+   git interpret-trailers --trailer review: --trailer fix=HEAD \
+   complex_message actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'with failing command using $ARG' '
+   git config trailer.fix.ifExists overwrite 
+   git config trailer.fix.command false \$ARG 
+   cat complex_message_body expected 
+   sed -e s/ Z\$/ / expected -EOF 
+   Fixes: Z
+   Acked-by= Z
+   Reviewed-by: Z
+   Signed-off-by: Z
+   Signed-off-by: A U Thor aut...@example.com
+   EOF
+   git interpret-trailers --trailer review: --trailer fix=HEAD \
+   complex_message actual 
+   test_cmp expected actual
+'
+
+test_expect_success 'with empty tokens' '
+   cat expected -EOF 
+
+   Signed-off-by: A U Thor aut...@example.com
+   EOF
+   git interpret-trailers --trailer : --trailer :test actual -EOF 
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'with command but no key' '
+   git config --unset trailer.sign.key 
+   cat expected -EOF 
+
+   sign: A U Thor aut...@example.com
+   EOF
+   git interpret-trailers actual -EOF 
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'with no command and no key' '
+   git config --unset trailer.review.key 
+   cat expected -EOF 
+
+   review: Junio
+   sign: A U Thor aut...@example.com
+   EOF
+   git interpret-trailers --trailer review:Junio actual -EOF 
+   EOF
+   test_cmp expected actual
+'
+
 test_done
--

[PATCH v11 01/11] trailer: add data structures and basic functions

We will use a doubly linked list to store all information
about trailers and their configuration.

This way we can easily remove or add trailers to or from
trailer lists while traversing the lists in either direction.

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 Makefile  |  1 +
 trailer.c | 49 +
 2 files changed, 50 insertions(+)
 create mode 100644 trailer.c

diff --git a/Makefile b/Makefile
index b4af1e2..ec90feb 100644
--- a/Makefile
+++ b/Makefile
@@ -871,6 +871,7 @@ LIB_OBJS += submodule.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
 LIB_OBJS += trace.o
+LIB_OBJS += trailer.o
 LIB_OBJS += transport.o
 LIB_OBJS += transport-helper.o
 LIB_OBJS += tree-diff.o
diff --git a/trailer.c b/trailer.c
new file mode 100644
index 000..db93a63
--- /dev/null
+++ b/trailer.c
@@ -0,0 +1,49 @@
+#include cache.h
+/*
+ * Copyright (c) 2013, 2014 Christian Couder chrisc...@tuxfamily.org
+ */
+
+enum action_where { WHERE_AFTER, WHERE_BEFORE };
+enum action_if_exists { EXISTS_ADD_IF_DIFFERENT, 
EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
+   EXISTS_ADD, EXISTS_OVERWRITE, EXISTS_DO_NOTHING };
+enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
+
+struct conf_info {
+   char *name;
+   char *key;
+   char *command;
+   enum action_where where;
+   enum action_if_exists if_exists;
+   enum action_if_missing if_missing;
+};
+
+struct trailer_item {
+   struct trailer_item *previous;
+   struct trailer_item *next;
+   const char *token;
+   const char *value;
+   struct conf_info conf;
+};
+
+static int same_token(struct trailer_item *a, struct trailer_item *b, int 
alnum_len)
+{
+   return !strncasecmp(a-token, b-token, alnum_len);
+}
+
+static int same_value(struct trailer_item *a, struct trailer_item *b)
+{
+   return !strcasecmp(a-value, b-value);
+}
+
+static int same_trailer(struct trailer_item *a, struct trailer_item *b, int 
alnum_len)
+{
+   return same_token(a, b, alnum_len)  same_value(a, b);
+}
+
+/* Get the length of buf from its beginning until its last alphanumeric 
character */
+static size_t alnum_len(const char *buf, size_t len)
+{
+   while (len  0  !isalnum(buf[len - 1]))
+   len--;
+   return len;
+}
-- 
1.9.1.636.g20d5f34


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v11 06/11] trailer: put all the processing together and print

This patch adds the process_trailers() function that
calls all the previously added processing functions
and then prints the results on the standard output.

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 trailer.c | 58 ++
 trailer.h |  6 ++
 2 files changed, 64 insertions(+)
 create mode 100644 trailer.h

diff --git a/trailer.c b/trailer.c
index 4ca9157..feadd3a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,5 +1,6 @@
 #include cache.h
 #include string-list.h
+#include trailer.h
 /*
  * Copyright (c) 2013, 2014 Christian Couder chrisc...@tuxfamily.org
  */
@@ -69,6 +70,26 @@ static void free_trailer_item(struct trailer_item *item)
free(item);
 }
 
+static void print_tok_val(const char *tok, const char *val)
+{
+   char c = tok[strlen(tok) - 1];
+   if (isalnum(c))
+   printf(%s: %s\n, tok, val);
+   else if (isspace(c) || c == '#')
+   printf(%s%s\n, tok, val);
+   else
+   printf(%s %s\n, tok, val);
+}
+
+static void print_all(struct trailer_item *first, int trim_empty)
+{
+   struct trailer_item *item;
+   for (item = first; item; item = item-next) {
+   if (!trim_empty || strlen(item-value)  0)
+   print_tok_val(item-token, item-value);
+   }
+}
+
 static void add_arg_to_input_list(struct trailer_item *in_tok,
  struct trailer_item *arg_tok)
 {
@@ -625,3 +646,40 @@ static int process_input_file(struct strbuf **lines,
 
return patch_start;
 }
+
+static void free_all(struct trailer_item **first)
+{
+   while (*first) {
+   struct trailer_item *item = remove_first(first);
+   free_trailer_item(item);
+   }
+}
+
+void process_trailers(const char *file, int trim_empty, struct string_list 
*trailers)
+{
+   struct trailer_item *in_tok_first = NULL;
+   struct trailer_item *in_tok_last = NULL;
+   struct trailer_item *arg_tok_first;
+   struct strbuf **lines;
+   int patch_start;
+
+   git_config(git_trailer_config, NULL);
+
+   lines = read_input_file(file);
+
+   /* Print the lines before the trailers */
+   patch_start = process_input_file(lines, in_tok_first, in_tok_last);
+
+   arg_tok_first = process_command_line_args(trailers);
+
+   process_trailers_lists(in_tok_first, in_tok_last, arg_tok_first);
+
+   print_all(in_tok_first, trim_empty);
+
+   free_all(in_tok_first);
+
+   /* Print the lines after the trailers as is */
+   print_lines(lines, patch_start, INT_MAX);
+
+   strbuf_list_free(lines);
+}
diff --git a/trailer.h b/trailer.h
new file mode 100644
index 000..8eb25d5
--- /dev/null
+++ b/trailer.h
@@ -0,0 +1,6 @@
+#ifndef TRAILER_H
+#define TRAILER_H
+
+void process_trailers(const char *file, int trim_empty, struct string_list 
*trailers);
+
+#endif /* TRAILER_H */
-- 
1.9.1.636.g20d5f34


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v11 09/11] trailer: execute command from 'trailer.name.command'

Let the user specify a command that will give on its standard output
the value to use for the specified trailer.

Signed-off-by: Christian Couder chrisc...@tuxfamily.org
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 trailer.c | 65 +++
 1 file changed, 65 insertions(+)

diff --git a/trailer.c b/trailer.c
index feadd3a..4d32b42 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1,5 +1,7 @@
 #include cache.h
 #include string-list.h
+#include run-command.h
+#include string-list.h
 #include trailer.h
 /*
  * Copyright (c) 2013, 2014 Christian Couder chrisc...@tuxfamily.org
@@ -14,11 +16,14 @@ struct conf_info {
char *name;
char *key;
char *command;
+   unsigned command_uses_arg : 1;
enum action_where where;
enum action_if_exists if_exists;
enum action_if_missing if_missing;
 };
 
+#define TRAILER_ARG_STRING $ARG
+
 struct trailer_item {
struct trailer_item *previous;
struct trailer_item *next;
@@ -60,6 +65,13 @@ static inline int contains_only_spaces(const char *str)
return !*s;
 }
 
+static inline void strbuf_replace(struct strbuf *sb, const char *a, const char 
*b)
+{
+   const char *ptr = strstr(sb-buf, a);
+   if (ptr)
+   strbuf_splice(sb, ptr - sb-buf, strlen(a), b, strlen(b));
+}
+
 static void free_trailer_item(struct trailer_item *item)
 {
free(item-conf.name);
@@ -403,6 +415,7 @@ static int git_trailer_config(const char *conf_key, const 
char *value, void *cb)
if (conf-command)
warning(_(more than one %s), conf_key);
conf-command = xstrdup(value);
+   conf-command_uses_arg = !!strstr(conf-command, 
TRAILER_ARG_STRING);
break;
case TRAILER_WHERE:
if (set_where(conf, value))
@@ -439,6 +452,45 @@ static int parse_trailer(struct strbuf *tok, struct strbuf 
*val, const char *tra
return 0;
 }
 
+static int read_from_command(struct child_process *cp, struct strbuf *buf)
+{
+   if (run_command(cp))
+   return error(running trailer command '%s' failed, 
cp-argv[0]);
+   if (strbuf_read(buf, cp-out, 1024)  1)
+   return error(reading from trailer command '%s' failed, 
cp-argv[0]);
+   strbuf_trim(buf);
+   return 0;
+}
+
+static const char *apply_command(const char *command, const char *arg)
+{
+   struct strbuf cmd = STRBUF_INIT;
+   struct strbuf buf = STRBUF_INIT;
+   struct child_process cp;
+   const char *argv[] = {NULL, NULL};
+   const char *result;
+
+   strbuf_addstr(cmd, command);
+   if (arg)
+   strbuf_replace(cmd, TRAILER_ARG_STRING, arg);
+
+   argv[0] = cmd.buf;
+   memset(cp, 0, sizeof(cp));
+   cp.argv = argv;
+   cp.env = local_repo_env;
+   cp.no_stdin = 1;
+   cp.out = -1;
+   cp.use_shell = 1;
+
+   if (read_from_command(cp, buf)) {
+   strbuf_release(buf);
+   result = xstrdup();
+   } else
+   result = strbuf_detach(buf, NULL);
+
+   strbuf_release(cmd);
+   return result;
+}
 
 static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
 {
@@ -469,6 +521,10 @@ static struct trailer_item *new_trailer_item(struct 
trailer_item *conf_item,
duplicate_conf(new-conf, conf_item-conf);
new-token = xstrdup(token_from_item(conf_item));
free(tok);
+   if (conf_item-conf.command_uses_arg || !val) {
+   new-value = apply_command(conf_item-conf.command, 
val);
+   free(val);
+   }
} else
new-token = tok;
 
@@ -530,12 +586,21 @@ static struct trailer_item 
*process_command_line_args(struct string_list *traile
struct trailer_item *arg_tok_first = NULL;
struct trailer_item *arg_tok_last = NULL;
struct string_list_item *tr;
+   struct trailer_item *item;
 
for_each_string_list_item(tr, trailers) {
struct trailer_item *new = create_trailer_item(tr-string);
add_trailer_item(arg_tok_first, arg_tok_last, new);
}
 
+   /* Add conf commands that don't use $ARG */
+   for (item = first_conf_item; item; item = item-next) {
+   if (item-conf.command  !item-conf.command_uses_arg) {
+   struct trailer_item *new = new_trailer_item(item, NULL, 
NULL);
+   add_trailer_item(arg_tok_first, arg_tok_last, new);
+   }
+   }
+
return arg_tok_first;
 }
 
-- 
1.9.1.636.g20d5f34


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is missing from Git v2.0

Jeff King wrote:
 On Fri, Apr 25, 2014 at 01:57:59PM -0500, Felipe Contreras wrote:
 
   Maybe I was not clear in my response, so let me try again. I do _not_
   necessarily agree that we need to move away from the name index.
  
  So you agree that the index is a bad name, and you agree staging area 
  is a
  better name, yet you don't agree we should move away from the term index?
 
 I don't agree that the index is a bad name, because that implies
 some objective level of bad.
 
 I do think the name staging area is fine, and I think it may even be
 better than index, if we were picking a name out of the blue.

 The way you have phrased it seems like you are trying to create a logical
 contradiction: A is bad, B is good, therefore we should move from A to B.

A _is_ bad, and B _is_ good. That's what I, and many people, have argued, and
that's what the vast majority of people have agreed.

 But that neglects the cost of moving.

And you neglect the benefits of moving.

It is very clear that the costs of moving are negligible in comparison to the
benefits.

But you *always* resist any change, so of course you wouldn't agree on change.
And that's why nothing ever changes.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'

From: Junio C Hamano gits...@pobox.com

 Christian Couder chrisc...@tuxfamily.org writes:
 
 +Help add RFC 822-like headers, called 'trailers', at the end of the
 +otherwise free-form part of a commit message.
 
 I think it is somewhat misleading to use the word headers like
 that.  'trailers' look similar to RFC-822-headers but they come at
 the end.  The sentence however reads as if they are headers that
 look like RFC 822.  Perhaps shuffling words like so:
 
   Help adding 'trailers' lines, that look similar to RFC 822
   e-mail headers, at the end of the ...
 
 would make it less confusing.

Ok, I made this change in v11.

 +Some configuration variables control the way the `token` arguments are
 +applied to the message and the way any existing trailer in the message
 +is changed. They also make it possible to automatically add some
 +trailers.
 +
 +By default, a 'token=value' or 'token:value' argument will be added
 +only if no trailer with the same (token, value) pair is already in the
 +message. The 'token' and 'value' parts will be trimmed to remove
 +starting and trailing whitespace, and the resulting trimmed 'token'
 +and 'value' will appear in the message like this:
 +
 +
 +token: value
 +
 
 Mental note: this does assume that the final output for the 'token'
 is to have a line label that is followed by a colon :, SP and
 the value.
 
 And the natural way to express that on the command line would be to
 say token: value, I would think, but let's just read on.
 
 +Note that 'trailers' do not follow and are not intended to follow many
 +rules that are in RFC 822. For example they do not follow the line
 +breaking rules, the encoding rules and probably many other rules.
 
 s/that are in RFC 822/for RFC 822 headers/.
 s/line breaking/line folding/. (see RFC 822, 3.1.1)

Ok, it's in v11 too.

 +OPTIONS
 +---
 +--trim-empty::
 +If the 'value' part of any trailer contains only whitespace,
 +the whole trailer will be removed from the resulting message.
 +
 +CONFIGURATION VARIABLES
 +---
 +
 +trailer.token.key::
 +This 'key' will be used instead of 'token' in the
 
 As `key` is something that is typed literally, it should be typeset
 as `key` in the descriptive text.

Ok, I used `key` in v11.

 I think other manpages spell the
 placeholder as `token` (or 'token', I am not sure which...).

I found mostly token, so I used that in v11.

 +trailer. After some alphanumeric characters, it can contain
 +some non alphanumeric characters like ':', '=' or '#' that will
 +be used instead of ':' to separate the token from the value in
 +the trailer, though the default ':' is more standard.
 
 I assume that this is for things like
 
   bug #538
 
 and the configuration would say something like:
 
   [trailer bug]
   key = bug #
 
 For completeness (of this example), the bog-standard s-o-b would
 look like
 
   Signed-off-by: Christian Couder chrisc...@tuxfamily.org
 
 and the configuration for it that spell the redundant key would
 be:
 
   [trailer Signed-off-by]
   key = Signed-off-by: 

Yeah, but you can use the following instead:

[trailer s-o-b]
key = Signed-off-by: 

The token and the key can be different.

 Am I reading the intention correctly?

Yeah, I think so.

 That is, when trailer.token.key is not defined, the value defaults
 to token:  (with one SP after the label and colon),

Yes.

 and when it
 is defined, the value can come directly after it.

The value can come directly after the key, only if the key ends with '#'.

If it ends with something else, except spaces, one SP will be added
between the key and the value.

Yeah, I made '#' special in the hope that it would be more compatible
with GitHub and other services that might also use '#'.

Thanks,
Christian.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Revert Stop starting pager recursively

2014-04-25 Thread Jörn Engel

On Fri, 25 April 2014 20:49:52 +0200, Matthieu Moy wrote:
 Jörn Engel jo...@logfs.org writes:
  On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote:
  
  This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84.
  
  Caused a usability regression for me and foul language for my coworkers.
 
  Ping.
 
 How do you solve the problem that the commit you revert was solving? The
 commit you propose to revert says in its message:
 
 git-column can be used as a pager for other git commands, something
 like this:
 
 GIT_PAGER=git -p column --mode='dense color' git -p branch
 
 The problem with this is that git -p column also has $GIT_PAGER set so
 the pager runs itself again as another pager. The end result is an
 infinite loop of forking.
 
 There's probably a solution, but you can't ignore the problem (or
 someone else will later try to solve the infinite loop and revert your
 commit, and so on ...).

Disclaimer: I never looked at git internals before this regression
forced me to and am likely talking out of my arse.

One approach is don't do that then.  Someone explicitly changed the
git pager to be git, which itself takes the git pager, etc.  That is
asking for infinite recursion and the original problem was that git
gave the user exactly what they asked for.

A second option is to add a --pager (or rather --no-pager) option to
the command line and allow the user to specify
GIT_PAGER=git --no-pager -p column --mode='dense color' git -p branch

A third option is to try to be smart and give the user what he wants,
not what he asked for.  If the pager happens to be git, unset
$GIT_PAGER, $PAGER and somehow disable core.pager.  Yeah, that will
turn nasty rather quickly.

A fourth option is to set an environment variable for the pager
process itself.  Disable paging similar to the original patch, but
make it conditional on we_are_the_pager(), not pager_in_use().

My preference is option four, but see disclaimer above.

Jörn

--
I've never met a human being who would want to read 17,000 pages of
documentation, and if there was, I'd kill him to get him out of the
gene pool.
-- Joseph Costello
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] l10n: de.po: improve hint for autocorrected command execution

2014-04-25 Thread Ralf Thielow

Signed-off-by: Ralf Thielow ralf.thie...@gmail.com
---
I'll queue this as part of the German l10n changes for
the next release.

 po/de.po | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/po/de.po b/po/de.po
index b777ef4..d143572 100644
--- a/po/de.po
+++ b/po/de.po
@@ -555,12 +555,12 @@ msgid 
 Continuing under the assumption that you meant '%s'
 msgstr 
 Warnung: Sie haben das nicht existierende Git-Kommando '%s' ausgeführt.\n
-Setze fort unter der Annahme, dass Sie '%s' gemeint haben
+Setze fort unter der Annahme, dass Sie '%s' gemeint haben.
 
 #: help.c:374
 #, c-format
 msgid in %0.1f seconds automatically...
-msgstr automatisch in %0.1f Sekunden...
+msgstr Automatische Ausführung in %0.1f Sekunden...
 
 #: help.c:381
 #, c-format
-- 
2.0.0.rc0.325.ge848631

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'

From: Michael Haggerty mhag...@alum.mit.edu

 On 04/08/2014 01:35 PM, Christian Couder wrote:
 
 The trailers are recognized in the input commit message using the
 following rules:
  - only lines that contains a ':' are considered trailers,
  - the trailer lines must all be next to each other,
  - after them it's only possible to have some lines that contain only spaces,
  - before them there must be at least one line with only spaces
 
 Thanks for all the explanation.  I think that most/all of this
 information should be included in the documentation.

Ok, I included the above rules in v11, but maybe not other pieces of
information that you might have wanted.

 +OPTIONS
 +---
 +--trim-empty::
 + If the 'value' part of any trailer contains only whitespace,
 + the whole trailer will be removed from the resulting message.

 Does this apply to existing trailers, new trailers, or both?
 
 Both.
 
 If it applies to existing trailers, then it seems a bit dangerous, in the
 sense that the command might end up changing trailers that are unrelated
 to the one that the command is trying to add.
 
 The command is not just for adding trailers.
 But there could be an option to just trim trailers that are added.
 
 Maybe that should be the *only* behavior of this option.
 
 Maybe there should be a trailer.token.trimEmpty config option.

One possible usage of the git interpret-trailers command that was
discussed in the early threads was the following:

1) You have a commit message template like the following:

-
***SUBJECT***

***MESSAGE***

Fixes: 
Cc: 
Cc: 
Reviewed-by: 
Reviewed-by: 
Signed-off-by: 
-

2) The user add some information when committing:

$ git commit --trailer Fixes:534 --trailer Signed-off-by: Michael 
mhag...@alum.mit.edu

3) git interpret-trailers is used automatically by git commit
without --trim-empty, and it is passed the --trailer arguments and the
commit message template, so the user is shown the result which is for
example the following:

-
***SUBJECT***

***MESSAGE***

Fixes: 534
Cc: 
Cc: 
Reviewed-by: 
Reviewed-by: 
Signed-off-by: Michael mhag...@alum.mit.edu
-

4) The user adds some information and the resulting message is for
example:

-
Doing foo and bar

And also baz.

Fixes: 534
Cc: 
Cc: Peff p...@peff.net
Reviewed-by: Junio gits...@pobox.com
Reviewed-by: 
Signed-off-by: Michael mhag...@alum.mit.edu
-

5) Then a post commit hook automatically uses git interpret-trailers
--trim-empty on the result, so the commit message is eventually the
following:

-
Doing foo and bar

And also baz.

Fixes: 534
Cc: Peff p...@peff.net
Reviewed-by: Junio gits...@pobox.com
Signed-off-by: Michael mhag...@alum.mit.edu
-

So I think it could be very useful to have --trim-empty work on all
the trailers, not just those passed as arguments.

 If a commit message containing trailer lines with separators other than
 ':' is input to the program, will it recognize them as trailer lines?
 
 No, '=' and '#' are not supported in the input message, only in the output.
 
 Do such trailer lines have to have the same separator as the one listed
 in this configuration setting to be recognized?
 
 No they need to have ':' as a separator.
 
 The reason why only ':' is supported is because it is the cannonical
 trailer separator and it could create problems with many input
 messages if other separators where supported.
 
 Maybe we could detect a special line like the following:
 
 # TRAILERS START
 
 in the input message and consider everyhting after that line as trailers.
 In this case it would be ok to accept other separators.
 
 It would be ugly to have to use such a line.  I think it would be
 preferable to be more restrictive about trailer separators than to
 require something like this.

The code is already very restrictive about trailer separators.

 From what you've said above, it sounds like your code might get confused
 with the following input commit message:
 
 This is the human-readable comment
 
 Foo: bar
 Fixes #123
 Plugh: xyzzy
 
 It seems to me that none of these lines would be accepted as trailers,
 because they include a non-trailer Fixes line (non-trailer in the
 sense that it doesn't use a colon separator).

Yeah, they would not be accepted because the code is very restrictive.

The following would be accepted:

 Foo: bar
 Fixes: 123
 Plugh: xyzzy

 I suppose that there is some compelling reason to allow non-colon
 separators here.  If not, it seems like it adds a lot of complexity and
 should maybe be omitted, or limited to only a few specific separators.
 
 Yeah, but in the early threads concerning this subject, someone said
 that GitHub for example uses bug #XXX.
 I will have a look again.
 
 Yes, that's true: GitHub recognizes strings like fixes #33 but not if
 there is an intervening colon like in fixes: #33.  OTOH GitHub
 recognizes

Re: [PATCH] l10n: de.po: translate 45 new messages

2014-04-25 Thread Thomas Rast

Ralf Thielow ralf.thie...@gmail.com writes:

 Translate 45 new messages came from git.pot update in 5e078fc
 (l10n: git.pot: v2.0.0 round 1 (45 new, 28 removed)).

Thanks for sending this with extra context, it really helps reviewing!

With the small changes below,

Acked-by: Thomas Rast t...@thomasrast.ch

  #: diffcore-rename.c:517
  msgid Performing inexact rename detection
 -msgstr 
 +msgstr Führe Erkennung für ungenaue Umbenennung aus

I don't have a better idea, but I wish we had a less unwieldy term for
inexact rename detection.

  #: wt-status.c:266
 -#, fuzzy, c-format
 +#, c-format
  msgid bug: unhandled unmerged status %x
 -msgstr Fehler: unbehandelter Differenz-Status %c
 +msgstr Fehler: unbehandelter Unmerged-Status %x

s/Fehler/Bug/?

I interpret Fehler in such a message as a user-caused problem, whereas
this message means the program doesn't handle a case that it really
should.

  #: builtin/commit.c:1131
 -#, fuzzy
  msgid Explicit paths specified without -i or -o; assuming --only paths...
  msgstr 
  Explizite Pfade ohne -i oder -o angegeben; unter der Annahme von --only 
  Pfaden...

Not new, but [I am] assuming --only paths should really be translated
as such, e.g.

  nehme --only an

(Leaving off the paths; I think it reads better without it even in
English.)

  #: builtin/gc.c:318
 -#, fuzzy, c-format
 +#, c-format
  msgid Auto packing the repository in background for optimum performance.\n
  msgstr 
 -Die Datenbank des Projektarchivs wird für eine optimale Performance 
 -komprimiert.\n
 +Die Datenbank des Repositories wird für eine optimale Performance 
 automatisch\n
 +im Hintergrund komprimiert.\n
  
  #: builtin/gc.c:320
  #, c-format
  msgid Auto packing the repository for optimum performance.\n
  msgstr 
  Die Datenbank des Projektarchivs wird für eine optimale Performance 
  komprimiert.\n

Nit: you added automatisch in the first one, but the second one
doesn't have it even though the difference between them is only in
background.

  #: builtin/gc.c:321
  #, c-format
  msgid See \git help gc\ for manual housekeeping.\n
 -msgstr 
 +msgstr Siehe \git help gc\ für manuelle Einstellungen.\n

für manuelles Aufräumen?

  #: builtin/notes.c:276
 -#, fuzzy, c-format
 +#, c-format
  msgid Cannot read note data from non-blob object '%s'.
 -msgstr Kann existierendes Objekt %s nicht lesen.
 +msgstr Kann Notiz-Daten von Nicht-Blob Objekt '%s' nicht lesen.

This would read better as

  Kann Notiz-Daten nicht von Nicht-Blob Objekt '%s' lesen.

since the key point is that we refuse any non-blob object, not that we
can't read it.

-- 
Thomas Rast
t...@thomasrast.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/11] refs.c: change ref_transaction_update() to do error checking and return status

Hi,

Ronnie Sahlberg wrote:

 Update ref_transaction_update() do some basic error checking and return
 true on error. Update all callers to check ref_transaction_update() for error.

Micronit: nonzero, not true.  (true tends to mean '1' while here we
have the usual error return of -1.  It's kind of annoying that C
doesn't have a nice way to distinguish between the usual int return of
0 for success and the usual bool return of true for success.)

Looks like a good change.  Some tiny nitpicks below.

[...]
 --- a/refs.h
 +++ b/refs.h
 @@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction 
 *transaction);
   * that the reference should have had before the update, or zeros if
   * it must not have existed beforehand.
   */
 -void ref_transaction_update(struct ref_transaction *transaction,
 +int ref_transaction_update(struct ref_transaction *transaction,

The comment above the prototype doesn't tell me:

When should the caller expect ref_transaction_update to return an
error?  What does an error mean: is it always a sign of a bug in the
caller, or can it be due to some other problem?  What guarantees does
the caller have about the state after an error --- is it just Things
will be in a sane state so you can free resources and exit, or will
the ref_transaction_update() have been essentially a no-op allowing
the caller to continue?

[...]
 --- a/refs.c
 +++ b/refs.c
 @@ -3327,19 +3327,24 @@ static struct ref_update *add_update(struct 
 ref_transaction *transaction,
   return update;
  }
  
 -void ref_transaction_update(struct ref_transaction *transaction,
 +int ref_transaction_update(struct ref_transaction *transaction,
   const char *refname,
   const unsigned char *new_sha1,
   const unsigned char *old_sha1,
   int flags, int have_old)
  {
 - struct ref_update *update = add_update(transaction, refname);
 + struct ref_update *update;
 +
 + if (have_old  !old_sha1)
 + return error(have_old is true but old_sha1 is NULL);

I agree with Michael that the error message should start with BUG:
so humans encountering this know to contact the list instead of
blaming themselves.

Returning error instead of die-ing seems like a nice thing that make
it easier to make git valgrind-clean some day.  Others might disagree
with me about whether that's worthwhile, but I think it's a good
change. :)

[...]
 --- a/builtin/update-ref.c
 +++ b/builtin/update-ref.c
 @@ -197,8 +197,10 @@ static const char *parse_cmd_update(struct strbuf 
 *input, const char *next)
   if (*next != line_termination)
   die(update %s: extra input: %s, refname, next);
  
 - ref_transaction_update(transaction, refname, new_sha1, old_sha1,
 -update_flags, have_old);
 + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
 +update_flags, have_old))
 + die(failed transaction update for %s, refname);

ref_transaction_update already printed an error, but of course that's
no guarantee that bugs in ref_transaction_update will not cause it
to fail without printing a message in the future.  And the extra
context for the error might be nice (but why not print refname in
the message from ref_transaction_update instead?).

Is the plan for ref_transaction_update to be able to fail for
other reasons some day?  What is the contract --- do we need a
human-readable, translatable message here, or is a this can't
happen BUG message fine?

I'd be fine with

die(BUG: failed transa...

or

/* ref_transaction_update already printed a message */
exit(128)

with a slight preference for the former, for what it's worth.

[...]
 @@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input, 
 const char *next)
   if (*next != line_termination)
   die(verify %s: extra input: %s, refname, next);
  
 - ref_transaction_update(transaction, refname, new_sha1, old_sha1,
 -update_flags, have_old);
 + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
 +update_flags, have_old))
 + die(failed transaction update for %s, refname);

Likewise.

Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/19] Use ref transactions from most callers

Ronnie Sahlberg wrote:

 This patch series changes most of the places where the ref functions for
 locking and writing refs to instead use the new ref transaction API.

Thanks.  Is this series based against mh/ref-transaction from next?

[...]
 I think I have covered all issues raised on the previous reviews and also
 done a bunch of cleanups and improvements to the transaction API.

Whoops, sorry I replied to an old message.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/19] Use ref transactions from most callers

On Fri, Apr 25, 2014 at 2:53 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 This patch series changes most of the places where the ref functions for
 locking and writing refs to instead use the new ref transaction API.

 Thanks.  Is this series based against mh/ref-transaction from next?

Yes, against mh/ref-transaction



 [...]
 I think I have covered all issues raised on the previous reviews and also
 done a bunch of cleanups and improvements to the transaction API.

 Whoops, sorry I replied to an old message.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string

Ronnie Sahlberg wrote:

 Let ref_transaction_commit return an optional error string that describes
 the transaction failure.  Start by returning the same error as update_ref_lock
 returns, modulo the initial error:/fatal: preamble.

s/returns/prints/?

 This will make it easier for callers to craft better error messages when
 a transaction call fails.

Interesting.  Can you give an example?  What kind of behavior are we
expecting in callers other than die()-ing or cleaning up and then
die()-ing?

I like this more than having the caller pass in a flag/callback/etc to
decide how noisy to be and whether to gracefully accept errors or exit.
So it seems like an improvement, but may always returning error()
would be better --- more context would help in clarifying this.

 --- a/refs.h
 +++ b/refs.h
 @@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction 
 *transaction,
   * Commit all of the changes that have been queued in transaction, as
   * atomically as possible.  Return a nonzero value if there is a
   * problem.  The ref_transaction is freed by this function.
 + * If error is non-NULL it will return an error string that describes
 + * why a commit failed. This string must be free()ed by the caller.
   */
  int ref_transaction_commit(struct ref_transaction *transaction,
 -const char *msg, enum action_on_err onerr);
 +const char *msg, char **err,
 +enum action_on_err onerr);

Is the idea that if I pass in a pointer err then
ref_transaction_commit will take the action described by onerr *and*
write its error message to err?

Probably squashing with patch 07 would make this easier to read (and
wouldn't require changing any messages at that point).

[...]
 --- a/refs.c
 +++ b/refs.c
[...]
 @@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction 
 *transaction,
  update-flags,
  update-type, onerr);
   if (!update-lock) {
 + if (err) {
 + const char *str = Cannot lock the ref '%s'.;
 + *err = xmalloc(PATH_MAX + 24);
 + snprintf(*err, PATH_MAX + 24, str,
 +  update-refname);
 + }

Might be clearer to use a helper similar to path.c::mkpathdup

char *aprintf(const char *fmt, ...)
{
char *result;
struct strbuf sb = STRBUF_INIT;
va_list args;

va_start(args, fmt);
strbuf_vaddf(sb, fmt, args);
va_end(args);

return strbuf_detach(sb);
}

or to have the caller pass in a pointer to strbuf instead of char *.

The rest looks good to me.

Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 04/19] refs.c: return error string from ref_update_reject_duplicates on failure

Ronnie Sahlberg wrote:

 --- a/refs.c
 +++ b/refs.c
 @@ -3393,6 +3393,7 @@ static int ref_update_compare(const void *r1, const 
 void *r2)
  }
  
  static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 + char **err,
   enum action_on_err onerr)
  {
   int i;
 @@ -3400,6 +3401,11 @@ static int ref_update_reject_duplicates(struct 
 ref_update **updates, int n,
   if (!strcmp(updates[i - 1]-refname, updates[i]-refname)) {
   const char *str =
   Multiple updates for ref '%s' not allowed.;
 + if (err) {
 + *err = xmalloc(PATH_MAX + 41);
 + snprintf(*err, PATH_MAX + 41, str,
 +  updates[i]-refname);
 + }

Same issues as the previous patch: it's too easy to get the buffer size
wrong when updating the message (or, worse, when making it
translatable).  aprintf or a strbuf should work better.

Otherwise seems sensible.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 05/19] update-ref.c: use the error string from _commit to print better message

Ronnie Sahlberg wrote:

 Call ref_transaction_commit with QUIET_ON_ERR and use the error string
 that is returned to print a better log message if/after the transaction
 fails.

Ah, so that's how the transition to a better API happens.  Makes sense.

(A mention of QUIET_ON_ERR in the patch that introduces the err
parameter could help, or feel free to ignore these comments, since it's
all well by the end of the series.)

 Update the tests to reflect that the log message is now slightly different
   fatal: update_ref failed: Cannot lock the ref 'some ref'
 versus from the previous
   fatal: Cannot lock the ref 'some ref'

Makes sense as a demo of what the new code allows, but is this error
message better?  The use of 'git update-ref' is an implementation
detail that the user doesn't need to know about.

I think I'd prefer the result of plain die(%s, err), even though
that's a no-op.

[...]
 +++ b/builtin/update-ref.c
[...]
 @@ -359,17 +360,18 @@ int cmd_update_ref(int argc, const char **argv, const 
 char *prefix)
   die(Refusing to perform update with empty message.);
  
   if (read_stdin) {
 - int ret;
   transaction = ref_transaction_begin();
 -
 + if (!transaction)
 + die(failed to update refs);

This can't happen (xcalloc is defined to die() on malloc failure).
If you want to protect against it anyway, die(BUG: ...)?

Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] remote-helpers: move out of contrib

2014-04-25 Thread Max Horn

Hi,

I am going to step away from this painful discussion and also this mailing
list, but I owed it to send one last reply with two of the problems I am
still seeing, simply in the hope that this will benefit some future
git-remote-hg users, but also to dispel any claims I was hoarding bugs to
somehow hurt Felipe.

Beyond that, I refuse to further discuss with Felipe on anything. It leads
nowhere, and he is so full of himself that it seems impossible to reason
with him. I will also refrain from countering everything he said, even
though I am sure he'll construe this as me admitting that he is right. I
don't care enough to try to keep up the flames *shrug*.  In the end,
everybody here can interpret this in whichever way they like.

Ah well, OK, I can't resist one last retort to one point Felipe wrote:

On 24.04.2014, at 02:23, Felipe Contreras felipe.contre...@gmail.com wrote:

 Max Horn wrote:
[...]
 
 Out of curiosity: How do you yourself use git-remote-hg in your daily work?
 
 I don't.

So you don't eat your own dog-food (at most you sometimes snack on it)?
That would explain a lot...



Now to the issues: The following results are based on git next at revision
e8486314780a4. In addition, I cobbled together fixes from Felipe's
repository, namely the commit [1] he claimed to have fixed the multi head
issue I mentioned, as well as the changes from the fc/remote/fixes branch on
his github repository [2].

It is very well possible that this is still not the latest and greatest, and
that I missed some important patch that changes everything -- it's hard to
tell given the multitude of branches in Felipe's repository. Anyway, to
avoid confusion, I put my merged version of the script into a Gist [3], so
if I made a mistake there, it should be easy to discover.

Given that, the following script (which Felipe already knows from his
issue tracker [4]) still causes a fast-import crash after which git pull
from the remote hg repository is impossible, and the user has no idea
how to recover.

-- 8 --
#!/bin/sh -ex
rm -rf hgrepo gitrepo

# Create a multi-head repository
hg init hgrepo
cd hgrepo
echo a  a  hg add a  hg commit -m a
echo b  b  hg add b  hg commit -m b
hg update 0
echo c  c  hg add c  hg commit -m c
cd ..

# Clone it via remote-hg
git clone hg::hgrepo gitrepo

cd gitrepo
git gc --prune=now
git pull  # error
-- 8 --

Which results in this:

WARNING: Branch 'default' has more than one head, consider merging
fatal: object not found: 61780798228d17af2d34fce4cfbdf35556832472
fast-import: dumping crash report to .git/fast_import_crash_78219
fatal: Error while running fast-import

Any subsequent pulls then give something like this:

WARNING: Branch 'default' has more than one head, consider merging
fatal: mark :6 not declared
fast-import: dumping crash report to .git/fast_import_crash_78834
fatal: Error while running fast-import


What happens here is that a hg branch with two heads is created; this
repository is cloned via git-remote-hg. Both heads of the hg branch are
imported to git, but only one is referenced by a git ref. We then prune, and
end up with a missing commit that is still referenced by the marks file.

Note that the next version I used has fc/transport-helper-sync-error-fix,
but this did not stop git fast-import from trashing the marks file. :-(



The second script is similar but uses a closed branch to trigger essentially
the same issue. Background: closed branches are ignored by git-remote-hg,
meaning that no git ref is created for them, which can again lead to a
commit without a git ref but referenced by the marks file(s). However,
reproducing the bug in this case is a bit more complicated, because the
problem is obscured by another bug (?): Namely, if a hg branch is closed,
then git-remote-hg starts ignoring it, but does not seem to remove the ref
created for that branch.  So, the git user will not see that the remote
branch was closed (which is a bug, I'd say); on the upside, since the ref is
still around, no dangling commit is produced.

But we can work around this by re-opening the hg branch at a different
position, i.e. as child of an unrelated commit, which does *not* have the
commits of the original branch as parents. If we do that, git-remote-hg
moves the git ref to point to the new branch tip, and again we end up with a
dangling commit (the git commit corresponding to the former hg branch tip).

Again, this is something me and also colleagues discovered in real life
usage. So don't be fooled by the somewhat convoluted test script. This *does*
happen.

-- 8 ---
#!/bin/sh -ex

rm -rf hgrepo gitrepo

# Create a repository with two branches
hg init hgrepo
cd hgrepo
echo a  a  hg add a  hg commit -m a
hg update 0
hg branch foobar
echo b  b  hg add b  hg commit -m b
hg update default
cd ..

# Clone it via remote-hg
git clone hg::hgrepo gitrepo
cd gitrepo
git gc --prune=now
git pull
cd ..

# close the branch
cd hgrepo
hg update foobar
hg commit --close-branch -m close branch
hg update

Re: [PATCH v3 06/19] refs.c: make update_ref_write to return error string on failure

Ronnie Sahlberg wrote:

 Change update_ref_write to also return an error string on failure.
 This makes the error avaialbel to ref_transaction_commit callers if the
 transaction failed dur to update_ref_sha1/write_ref_sha1 failures.

Nits:

 * available
 * during

Probably should come right after or before patch 3.  Same nit as patch
3 about using asprintf or passing in a pointer to strbuf.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 07/19] refs.c: remove the onerr argument to ref_transaction_commit

Ronnie Sahlberg wrote:

 Since we now pass meaningful error messages back from ref_transaction_commit
 on failures, we no longer need to provide a onerr argument.

Yay!  More precisely, now that all callers use
UPDATE_REFS_QUIET_ON_ERR there's no need to support any other
behavior.

Thanks for cleaning up the error handling here.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

What's cooking in git.git (Apr 2014, #08; Fri, 25)

2014-04-25 Thread Junio C Hamano

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.

The tip of the 'master' branch is at v2.0.0-rc1.  Last minute fixes
to newly added code keep flowing in, which is good.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to master]

* db/make-with-curl (2014-04-15) 2 commits
  (merged to 'next' on 2014-04-16 at b9c8527)
 + Makefile: allow static linking against libcurl
 + Makefile: use curl-config to determine curl flags

 Ask curl-config how to link with the curl library, instead of
 having only a limited configurability knobs in the Makefile.


* fc/transport-helper-sync-error-fix (2014-04-21) 6 commits
  (merged to 'next' on 2014-04-21 at f53a08a)
 + t5801 (remote-helpers): cleanup environment sets
 + transport-helper: fix sync issue on crashes
 + transport-helper: trivial cleanup
 + transport-helper: propagate recvline() error pushing
 + remote-helpers: make recvline return an error
 + transport-helper: remove barely used xchgline()

 Make sure the marks are not written out when the transport helper
 did not finish happily, to avoid marks file that is out of sync
 with the reality.


* jk/pack-bitmap (2014-04-22) 1 commit
  (merged to 'next' on 2014-04-22 at ba58737)
 + ewah_bitmap.c: do not assume size_t and eword_t are the same size

 A last minute (and hopefully the last) fix to avoid coredumps due
 to an incorrect pointer arithmetic.

--
[New Topics]

* ep/shell-command-substitution (2014-04-23) 13 commits
 - p5302-pack-index.sh: use the $( ... ) construct for command substitution
 - lib-gpg.sh: use the $( ... ) construct for command substitution
 - lib-cvs.sh: use the $( ... ) construct for command substitution
 - lib-credential.sh: use the $( ... ) construct for command substitution
 - git-web--browse.sh: use the $( ... ) construct for command substitution
 - git-stash.sh: use the $( ... ) construct for command substitution
 - git-rebase.sh: use the $( ... ) construct for command substitution
 - git-rebase--merge.sh: use the $( ... ) construct for command substitution
 - git-pull.sh: use the $( ... ) construct for command substitution
 - appp.sh: use the $( ... ) construct for command substitution
 - t7900-subtree.sh: use the $( ... ) construct for command substitution
 - test-gitmw-lib.sh: use the $( ... ) construct for command substitution
 - t9365-continuing-queries.sh: use the $( ... ) construct for command 
substitution

 Adjust shell scripts to use $(cmd) instead of `cmd`.

 Will merge to 'next' and keep it there for the remainder of the cycle.


* ib/test-selectively-run (2014-04-23) 3 commits
 - test-lib: '--run' to run only specific tests
 - test-lib: tests skipped by GIT_SKIP_TESTS say so
 - test-lib: Document short options in t/README

 Allow specifying only certain individual test pieces to be run
 using a range notation (e.g. t1234-test.sh --run='1-4 6 8 9-').


* mm/mediawiki-encoding-fix (2014-04-23) 2 commits
 - git-remote-mediawiki: fix encoding issue for UTF-8 media files
 - git-remote-mediawiki: allow stop/start-ing the test server

 Will merge to 'next' and keep it there for the remainder of the cycle.


* mw/symlinks (2014-04-24) 1 commit
  (merged to 'next' on 2014-04-25 at 20b2af6)
 + setup: fix windows path buffer over-stepping

 A finishing touch fix to a new change already in 'master'.

 Will merge to 'master' by -rc2.


* sk/tag-contains-wo-recursion (2014-04-25) 1 commit
  (merged to 'next' on 2014-04-25 at f320750)
 + git tag --contains: avoid stack overflow

 Will keep in 'next' for the remainder of the cycle.

--
[Stalled]

* rs/ref-transaction (2014-04-22) 14 commits
 . SQUASH???
 . refs.c: change update_ref to use a transaction
 . walker.c: use ref transaction for ref updates
 . branch.c: use ref transaction for all ref updates
 . fast-import.c: change update_branch to use ref transactions
 . sequencer.c: use ref transactions for all ref updates
 . commit.c: use ref transactions for updates
 . replace.c: use the ref transaction functions for updates
 . tag.c: use ref transactions when doing updates
 . refs.c: ref_transaction_delete to check for error and return status
 . refs.c: change ref_transaction_create to do error checking and return status
 . refs.c: change ref_transaction_update() to do error checking and return 
status
 . refs.c: use a single exit path from transaction commit and handle onerr
 . refs.c: constify the sha arguments for ref_transaction_create|delete|update
 (this branch uses mh/ref-transaction.)

 Updates most of the callsites to write_sha1_ref(), the low-level
 mechanism to update a ref, to use the ref-transaction API.

 Seems to break the dumb walker test (t5550) when

[ANNOUNCE] Git v2.0.0-rc1

2014-04-25 Thread Junio C Hamano

A release candidate Git v2.0.0-rc1 is now available for testing
at the usual places.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/testing/

The following public repositories all have a copy of the 'v2.0.0-rc1'
tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = https://code.google.com/p/git-core/
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

Git v2.0 Release Notes (draft)
==

Backward compatibility notes


When git push [$there] does not say what to push, we have used the
traditional matching semantics so far (all your branches were sent
to the remote as long as there already are branches of the same name
over there).  In Git 2.0, the default is now the simple semantics,
which pushes:

 - only the current branch to the branch with the same name, and only
   when the current branch is set to integrate with that remote
   branch, if you are pushing to the same remote as you fetch from; or

 - only the current branch to the branch with the same name, if you
   are pushing to a remote that is not where you usually fetch from.

You can use the configuration variable push.default to change
this.  If you are an old-timer who wants to keep using the
matching semantics, you can set the variable to matching, for
example.  Read the documentation for other possibilities.

When git add -u and git add -A are run inside a subdirectory
without specifying which paths to add on the command line, they
operate on the entire tree for consistency with git commit -a and
other commands (these commands used to operate only on the current
subdirectory).  Say git add -u . or git add -A . if you want to
limit the operation to the current directory.

git add path is the same as git add -A path now, so that
git add dir/ will notice paths you removed from the directory and
record the removal.  In older versions of Git, git add path used
to ignore removals.  You can say git add --ignore-removal path to
add only added or modified paths in path, if you really want to.

The -q option to git diff-files, which does *NOT* mean quiet,
has been removed (it told Git to ignore deletion, which you can do
with git diff-files --diff-filter=d).

git request-pull lost a few heuristics that often led to mistakes.

The default prefix for git svn has changed in Git 2.0.  For a long
time, git svn created its remote-tracking branches directly under
refs/remotes, but it now places them under refs/remotes/origin/ unless
it is told otherwise with its --prefix option.


Updates since v1.9 series
-

UI, Workflows  Features

 * The multi-mail post-receive hook (in contrib/) has been updated
   to a more recent version from the upstream.

 * git gc --aggressive learned --depth option and
   gc.aggressiveDepth configuration variable to allow use of a less
   insane depth than the built-in default value of 250.

 * git log learned the --show-linear-break option to show where a
   single strand-of-pearls is broken in its output.

 * The rev-parse --parseopt mechanism used by scripted Porcelains to
   parse command line options and to give help text learned to take
   the argv-help (the placeholder string for an option parameter,
   e.g. key-id in --gpg-sign=key-id).

 * The pattern to find where the function begins in C/C++ used in
   diff and grep -p have been updated to help C++ source better.

 * git rebase learned to interpret a lone - as @{-1}, the
   branch that we were previously on.

 * git commit --cleanup=mode learned a new mode, scissors.

 * git tag --list output can be sorted using version sort with
   --sort=version:refname.

 * Discard the accumulated heuristics to guess from which branch the
   result wants to be pulled from and make sure what the end user
   specified is not second-guessed by git request-pull, to avoid
   mistakes.  When you pushed out your 'master' branch to your public
   repository as 'for-linus', use the new master:for-linus syntax to
   denote the branch to be pulled.

 * git grep learned to behave in a way similar to native grep when
   -h (no header) and -c (count) options are given.

 * git push via transport-helper interface (e.g. remote-hg) has
   been updated to allow ref deletion in a way similar to the natively
   supported transports.

 * The simple mode is the default for git push.

 * git add -u and git add -A, when run without any pathspec, is a
   tree-wide operation even when run inside a subdirectory of a
   working tree.

 * git add path is the same as git add -A path now.

 * core.statinfo configuration variable, which is a
   never-advertised synonym to core.checkstat, has been removed.

 * The -q option to git diff-files, which does *NOT* mean
   quiet, has been removed (it told

Re: [PATCH 04/14] appp.sh: use the $( ... ) construct for command substitution

2014-04-25 Thread brian m. carlson

On Wed, Apr 23, 2014 at 06:43:56AM -0700, Elia Pinto wrote:
 @@ -25,11 +25,11 @@ fi
  
  cd -  /dev/null
  
 -SUBJECT=`sed -n -e '/^Subject: /p' ${PATCH}`
 -HEADERS=`sed -e '/^'${SEP}'$/,$d' $1`
 -BODY=`sed -e 1,/${SEP}/d $1`
 -CMT_MSG=`sed -e '1,/^$/d' -e '/^---$/,$d' ${PATCH}`
 -DIFF=`sed -e '1,/^---$/d' ${PATCH}`
 +SUBJECT=$(sed -n -e '/^Subject: /p' ${PATCH})
 +HEADERS=$(sed -e '/^'${SEP}'$/,$d' $1)
 +BODY=$(sed -e 1,/${SEP}/d $1)
 +CMT_MSG=$(sed -e '1,/^$/d' -e '/^---$/,$d' ${PATCH})
 +DIFF=$(sed -e '1,/^---$/d' ${PATCH})
  
  CCS=`echo -e $CMT_MSG\n$HEADERS | sed -n -e 's/^Cc: \(.*\)$/\1,/gp' \
   -e 's/^Signed-off-by: \(.*\)/\1,/gp'`

It looks like you may have missed a usage here due to the line break.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: Digital signature

Re: [PATCH v3 11/19] tag.c: use ref transactions when doing updates

On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
 Change tag.c to use ref transactions for all ref updates.
 
 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 ---
  builtin/tag.c | 14 --
  1 file changed, 8 insertions(+), 6 deletions(-)
 
 diff --git a/builtin/tag.c b/builtin/tag.c
 index 40356e3..dd53fb4 100644
 --- a/builtin/tag.c
 +++ b/builtin/tag.c
 @@ -488,7 +488,6 @@ int cmd_tag(int argc, const char **argv, const char 
 *prefix)
   struct strbuf ref = STRBUF_INIT;
   unsigned char object[20], prev[20];
   const char *object_ref, *tag;
 - struct ref_lock *lock;
   struct create_tag_options opt;
   char *cleanup_arg = NULL;
   int annotate = 0, force = 0, lines = -1;
 @@ -496,6 +495,8 @@ int cmd_tag(int argc, const char **argv, const char 
 *prefix)
   const char *msgfile = NULL, *keyid = NULL;
   struct msg_arg msg = { 0, STRBUF_INIT };
   struct commit_list *with_commit = NULL;
 + struct ref_transaction *transaction;
 + char *err = NULL;
   struct option options[] = {
   OPT_CMDMODE('l', list, cmdmode, N_(list tag names), 'l'),
   { OPTION_INTEGER, 'n', NULL, lines, N_(n),
 @@ -641,11 +642,12 @@ int cmd_tag(int argc, const char **argv, const char 
 *prefix)
   if (annotate)
   create_tag(object, tag, buf, opt, prev, object);
  
 - lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
 - if (!lock)
 - die(_(%s: cannot lock the ref), ref.buf);
 - if (write_ref_sha1(lock, object, NULL)  0)
 - die(_(%s: cannot update the ref), ref.buf);
 + transaction = ref_transaction_begin();
 + if (!transaction ||
 + ref_transaction_update(transaction, ref.buf, object, prev,
 +0, !is_null_sha1(prev)) ||
 + ref_transaction_commit(transaction, NULL, err))
 +   die(_(%s: cannot update the ref: %s), ref.buf, err);

Formatting nit: die() should be indented by two TABs.

   if (force  !is_null_sha1(prev)  hashcmp(prev, object))
   printf(_(Updated tag '%s' (was %s)\n), tag, 
 find_unique_abbrev(prev, DEFAULT_ABBREV));
  
 

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 12/19] replace.c: use the ref transaction functions for updates

On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
 Update replace.c to use ref transactions for updates.
 
 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 ---
  builtin/replace.c | 14 --
  1 file changed, 8 insertions(+), 6 deletions(-)
 
 diff --git a/builtin/replace.c b/builtin/replace.c
 index b62420a..cf0f56d 100644
 --- a/builtin/replace.c
 +++ b/builtin/replace.c
 @@ -129,7 +129,8 @@ static int replace_object(const char *object_ref, const 
 char *replace_ref,
   unsigned char object[20], prev[20], repl[20];
   enum object_type obj_type, repl_type;
   char ref[PATH_MAX];
 - struct ref_lock *lock;
 + struct ref_transaction *transaction;
 + char *err = NULL;
  
   if (get_sha1(object_ref, object))
   die(Failed to resolve '%s' as a valid ref., object_ref);
 @@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const 
 char *replace_ref,
   else if (!force)
   die(replace ref '%s' already exists, ref);
  
 - lock = lock_any_ref_for_update(ref, prev, 0, NULL);
 - if (!lock)
 - die(%s: cannot lock the ref, ref);
 - if (write_ref_sha1(lock, repl, NULL)  0)
 - die(%s: cannot update the ref, ref);
 + transaction = ref_transaction_begin();
 + if (!transaction ||
 + ref_transaction_update(transaction, ref, repl, prev,
 +0, !is_null_sha1(prev)) ||
 + ref_transaction_commit(transaction, NULL, err))
 +   die(_(%s: failed to replace ref: %s), ref, err);

die() should be indented by two TABs.

  
   return 0;
  }
 

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: gitignore vs. exclude vs assume-unchanged?

2014-04-25 Thread alex


Andrew Ardill andrew.ard...@gmail.com writes:

As a data point, I have seen people add .gitignore to their
.gitignore file, as they don't want to share the file.


Right, I've seen that too. It confused the heck out of me. It only lends 
credence to my point about the docs. Those users want the functionality 
of a pattern in '$GIT_DIR/info/exclude', but haven't been able to figure 
it out easily enough. They've just heard about .gitignore, so they're 
using that. Yes, it's all there in the docs if you read it several 
times, and you already know what you're looking at, but not in a 
terribly accessible, best practices, advice from a smart friend who's 
been through it all already kind of way.



... The introduction does specifically mention 'gitignore'
files, but that seems to be due to all the ignore files
($HOME/.config/git/ignore, $GIT_DIR/info/exclude, .gitignore) being
classified as 'gitignore' files.


Yes, the 'gitignore' versus '.gitignore' distinction is hopelessly 
subtle. It is very easy for a newcomer to think these are exactly the 
same thing. I certainly did.


Finally, it's a little confusing that one of the files is called 
'exclude'.


It would be great to rename it to 'ignore'; $GIT_DIR/info/exclude -
$GIT_DIR/info/ignore. Is there any reason this shouldn't be done?


Well, yes: semantics. Since they do slightly different things, they 
should have different names. It makes reference and teaching much 
easier. In fact, if a renaming were to occur, I would actually prefer 
even better semantics:


.gitignore - .shared-ignore

$GIT_DIR/info/exclude - $GIT_DIR/info/local-ignore

These suggested names could perhaps be improved on. But if anything, the 
names need to be more different, not less. Users should be able to 
instantly know what a speaker is talking about without having to 
doublecheck and ask if by git-ignore, the speaker really meant git 
ignore or dot-gitignore.


Thanks,
Alex

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] commit: do not complain of empty messages from -C

When we pick another commit's message, we die() immediately
if we find that it's empty and we are not going to run an
editor (i.e., when running -C instead of -c).  However,
this check is redundant and harmful.

It's redundant because we will already notice the empty
message later, after we would have run the editor, and die
there (just as we would for a regular, not -C case, where
the user provided an empty message in the editor).

It's harmful for a few reasons:

  1. It does not respect --allow-empty-message. As a result,
 a git rebase -i cannot pick such a commit. So you
 cannot even go back in time to fix it with a reword
 or edit instruction.

  2. It does not take into account other ways besides the
 editor to modify the message. For example, git commit
 -C empty-commit -m foo could take the author
 information from empty-commit, but add a message to it.
 There's more to do to make that work correctly (and
 right now we explicitly forbid -C with -m), but this
 removes one roadblock.

  3. The existing check is not enough to prevent segfaults.
 We try to find the \n\n header/body boundary in the
 commit. If it is at the end of the string (i.e., no
 body), _or_ if we cannot find it at all (i.e., a
 truncated commit object), we consider the message
 empty. With -C, that's OK; we die in either case. But
 with -c, we continue on, and in the case of a
 truncated commit may end up dereferencing NULL+2.

Signed-off-by: Jeff King p...@peff.net
---
I care most about the rebase -i thing, especially because it is the
primary method for fixing old mistakes. The segfault fix is a nice
bonus.

The git commit -C empty -m foo thing might be nice, but I don't plan
to work on it further. The semantics would need to be figured out (does
it append or replace?), and you can always just use -c to fire up an
actual editor and write the new content there.

 builtin/commit.c  |  5 ++---
 t/t7500-commit.sh | 11 ++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..65c069d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -650,9 +650,8 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
} else if (use_message) {
char *buffer;
buffer = strstr(use_message_buffer, \n\n);
-   if (!use_editor  (!buffer || buffer[2] == '\0'))
-   die(_(commit has empty message));
-   strbuf_add(sb, buffer + 2, strlen(buffer + 2));
+   if (buffer)
+   strbuf_add(sb, buffer + 2, strlen(buffer + 2));
hook_arg1 = commit;
hook_arg2 = use_message;
} else if (fixup_message) {
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index bdc1f29..116885a 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -223,7 +223,8 @@ test_expect_success 'Commit without message is allowed with 
--allow-empty-messag
git add foo 
empty 
git commit --allow-empty-message empty 
-   commit_msg_is 
+   commit_msg_is  
+   git tag empty-message-commit
 '
 
 test_expect_success 'Commit without message is no-no without 
--allow-empty-message' '
@@ -240,6 +241,14 @@ test_expect_success 'Commit a message with 
--allow-empty-message' '
commit_msg_is hello there
 '
 
+test_expect_success 'commit -C empty respects --allow-empty-message' '
+   echo more foo 
+   git add foo 
+   test_must_fail git commit -C empty-message-commit 
+   git commit -C empty-message-commit --allow-empty-message 
+   commit_msg_is 
+'
+
 commit_for_rebase_autosquash_setup () {
echo first content line foo 
git add foo 
-- 
1.9.1.656.ge8a0637
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 16/19] branch.c: use ref transaction for all ref updates

On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
 Change create_branch to use a ref transaction when creating the new branch.
 ref_transaction_create will check that the ref does not already exist and fail
 otherwise meaning that we no longer need to keep a lock on the ref during the
 setup_tracking. This simplifies the code since we can now do the transaction
 in one single step.
 
 If the forcing flag is false then use ref_transaction_create since this will
 fail if the ref already exist. Otherwise use ref_transaction_update.
 
 This also fixes a race condition in the old code where two concurrent
 create_branch could race since the lock_any_ref_for_update/write_ref_sha1
 did not protect against the ref already existsing. I.e. one thread could end 
 up
 overwriting a branch even if the forcing flag is false.
 
 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 ---
  branch.c | 39 +--
  1 file changed, 25 insertions(+), 14 deletions(-)
 
 diff --git a/branch.c b/branch.c
 index 660097b..23cde1e 100644
 --- a/branch.c
 +++ b/branch.c
 @@ -226,7 +226,6 @@ void create_branch(const char *head,
  int force, int reflog, int clobber_head,
  int quiet, enum branch_track track)
  {
 - struct ref_lock *lock = NULL;
   struct commit *commit;
   unsigned char sha1[20];
   char *real_ref, msg[PATH_MAX + 20];
 @@ -285,15 +284,6 @@ void create_branch(const char *head,
   die(_(Not a valid branch point: '%s'.), start_name);
   hashcpy(sha1, commit-object.sha1);
  
 - if (!dont_change_ref) {
 - lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
 - if (!lock)
 - die_errno(_(Failed to lock ref for update));
 - }
 -
 - if (reflog)
 - log_all_ref_updates = 1;
 -
   if (forcing)
   snprintf(msg, sizeof msg, branch: Reset to %s,
start_name);
 @@ -301,13 +291,34 @@ void create_branch(const char *head,
   snprintf(msg, sizeof msg, branch: Created from %s,
start_name);
  
 + if (reflog)
 + log_all_ref_updates = 1;
 +
 + if (!dont_change_ref) {
 + struct ref_transaction *transaction;
 + char *err = NULL;
 +
 + transaction = ref_transaction_begin();
 + if (forcing) {
 + if (!transaction ||
 + ref_transaction_update(transaction, ref.buf, sha1,
 +NULL, 0, 0) ||
 + ref_transaction_commit(transaction, msg, err))
 +   die_errno(_(%s: failed to write ref: %s),
 + ref.buf, err);
 + } else {
 + if (!transaction ||
 + ref_transaction_create(transaction, ref.buf, sha1,
 +0) ||
 + ref_transaction_commit(transaction, msg, err))
 +   die_errno(_(%s: failed to write ref: %s),
 + ref.buf, err);
 + }

You've got some indentation problems above.

But actually, there seems like a lot of duplicated code here.  Couldn't
you instead do a single block with have_old set based on forcing:

ref_transaction_update(transaction, ref.buf, sha1,
   null_sha1, 0, !forcing)

?

 + }
 +
   if (real_ref  track)
   setup_tracking(ref.buf + 11, real_ref, track, quiet);
  
 - if (!dont_change_ref)
 - if (write_ref_sha1(lock, sha1, msg)  0)
 - die_errno(_(Failed to write ref));
 -
   strbuf_release(ref);
   free(real_ref);
  }
 


-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Apr 2014, #08; Fri, 25)

On Fri, Apr 25, 2014 at 03:50:26PM -0700, Junio C Hamano wrote:

 * jk/external-diff-use-argv-array (2014-04-21) 6 commits
   (merged to 'next' on 2014-04-22 at e6d92d7)
  + run_external_diff: refactor cmdline setup logic
  + run_external_diff: hoist common bits out of conditional
  + run_external_diff: drop fflush(NULL)
  + run_external_diff: clean up error handling
  + run_external_diff: use an argv_array for the environment
  + run_external_diff: use an argv_array for the command line
 
  Code clean-up.
 
  Will keep in 'next' for the remainder of the cycle.

The first one does fix a possible stack overflow (albeit of one NULL,
not arbitrary content, so I don't think it's exploitable). We may want
to do:

diff --git a/diff.c b/diff.c
index 54d5308..a03744b 100644
--- a/diff.c
+++ b/diff.c
@@ -2894,7 +2894,7 @@ static void run_external_diff(const char *pgm,
  int complete_rewrite,
  struct diff_options *o)
 {
-   const char *spawn_arg[10];
+   const char *spawn_arg[11];
int retval;
const char **arg = spawn_arg[0];
struct diff_queue_struct *q = diff_queued_diff;

as a fix for maint/2.0.0 in the interim. I can write a commit message
for that if you're interested.

 * fc/publish-vs-upstream (2014-04-21) 8 commits
  - sha1_name: add support for @{publish} marks
  - sha1_name: simplify track finding
  - sha1_name: cleanup interpret_branch_name()
  - branch: display publish branch
  - push: add --set-publish option
  - branch: add --set-publish-to option
  - Add concept of 'publish' branch
  - t5516 (fetch-push): fix test restoration
 
  Add branch@{publish}; it seems that this is somewhat different from
  Ram and Peff started working on.  There were many discussion
  messages going back and forth but it does not appear that the
  design issues have been worked out among participants yet.

If you are waiting on me, I do not have much else to say on this topic.
@{publish} as specified by Felipe is not useful to me, and I would
continue to pursue @{push} separately as the remote-tracking branch of
where you would push to. I think there is room for both concepts.

As for the patches themselves, I have not reviewed them carefully, and
would prefer not to. As I mentioned before, though, I would prefer the
short @{p} not be taken for @{publish} until it has proven itself.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Updated C# userdiff patterns.

2014-04-25 Thread Marius Ungureanu

New keywords: foreach, break, in, try, finally, as, is, typeof, var,
default, fixed, checked, unchecked, this, lock, readonly, unsafe,
ref, out, base, null, delegate, continue.

Removed keywords: instanceof. It's only in Java.
Moved keywords to happen before modifier parsing, as matching a keyword
will stop modifiers from being matched.

Added method modifiers: extern, abstract.

Added properties modifiers: abstract.

Added parsing of events and delegates, which are like properties, but
take an extra keyword.

The reasoning behind adding unsafe to keywords is being also a
statement that can happen inline in code to mention blocks which are
unsafe. Also, delegates are not necessarily declared in class bodies,
but can also happen inline in other functions.

Keywords are based on MSDN docs.

Signed-off-by: Marius Ungureanu marius.ungure...@xamarin.com
---
 userdiff.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/userdiff.c b/userdiff.c
index fad52d6..7612c5d 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -133,14 +133,14 @@ PATTERNS(cpp,
 |[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lLuU]*
 |[-+*/%^|=!]=|--|\\+\\+|=?|=?||\\|\\||::|-\\*?|\\.\\*),
 PATTERNS(csharp,
-/* Keywords */
-!^[ 
\t]*(do|while|for|if|else|instanceof|new|return|switch|case|throw|catch|using)\n
 /* Methods and constructors */
-^[ 
\t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe)[
 \t]+)*[][@.~_[:alnum:]]+[ \t]+[@._[:alnum:]]+[ \t]*\\(.*\\))[ \t]*$\n
-/* Properties */
-^[ 
\t]*(((static|public|internal|private|protected|new|virtual|sealed|override|unsafe)[
 \t]+)*[][@.~_[:alnum:]]+[ \t]+[@._[:alnum:]]+)[ \t]*$\n
+^[ 
\t]*(((abstract|extern|internal|new|override|private|protected|public|sealed|static|unsafe|virtual)[
 \t]+)*[][@.~_[:alnum:]]+[ \t]+[@._[:alnum:]]+[ \t]*\\(.*\\))[ \t]*$\n
+/* Properties, events, delegates */
+^[ 
\t]*(((abstract|internal|new|override|private|protected|public|sealed|static|unsafe|virtual)[
 \t]+)*((delegate|event)[ \t]+)*[][@.~_[:alnum:]]+[ \t]+[@._[:alnum:]]+)[ 
\t]*$\n
 /* Type definitions */
-^[ 
\t]*(((static|public|internal|private|protected|new|unsafe|sealed|abstract|partial)[
 \t]+)*(class|enum|interface|struct)[ \t]+.*)$\n
+^[ 
\t]*(((abstract|internal|new|override|partial|private|protected|public|sealed|static|unsafe)[
 \t]+)*(class|enum|interface|struct)[ \t]+.*)$\n
+/* Keywords */
+!^[ 
\t]*(as|base|break|case|catch|checked|continue|default|delegate|do|else|finally|fixed|for|foreach|if|in|is|lock|new|null|out|readonly|ref|return|switch|this|throw|try|typeof|unchecked|unsafe|using|var|while)\n
 /* Namespace */
 ^[ \t]*(namespace[ \t]+.*)$,
 /* -- */
-- 
1.8.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 17/19] refs.c: change update_ref to use a transaction