RE: feature request: git svn dommit --preserve-timestamps

[Randall S. Becker]

Somewhen near June 10, 2016 9:40 PM, Eric Wong wrote:
> Peter Münster  wrote:
> > On Tue, Jun 07 2016, Eric Wong wrote:
> > > Peter Münster  wrote:
> > >> It would be nice, if timestamps could be preserved when rewriting
> > >> the git-log.
> > >
> > > Unfortunately, last I checked (a long time ago!), explicitly setting
> > > revprops might require SVN administrators to enable the feature for
> > > the repo.
> >
> > Not the svn-log, only the git-log.
> 
> The git log after dcommit is tied to the SVN log, so git-svn can only reflect
> changes which appear in SVN.
> 
>   Sidenote: The convention is reply-to-all on lists like
>   this one which do not require subscription to post.
>   It prevents the list from being a single-point-of-failure
>   or censorship.
> 
> > > It's been a while and I'm not up-to-date with the latest SVN.
> > > Maybe there's a newer/easier way you could give us details about :)
> >
> > No, sorry. I don't care about the svn-log.
> 
> Unfortunately, you would have to care about svn log as long as SVN exists in
> your workflow and you need to interact with SVN users.
> 
> git svn tries hard to work transparently and as close to the behavior of the
> upstream SVN repo as possible.

Having had to deal with this in pure git without factoring in git svn, this 
seems to be is a matter of policy rather than technical requirement. Various 
customers of mine have decided that using the commit time as a uniform 
timestamp to be applied to all files pulled in the specific commit, is the way 
to go when doing continuous integration. The solution that we ended up with was 
a step in our Jenkins build jobs that would set the timestamp of all files 
associated with the specific commit to the time of the commit itself. Any 
commit not part of the commit that changed that state of the repository was 
untouched. This became arbitrarily complex when the job was impacted by 
multiple commits, but the general consensus of those who made the decisions was 
to apply all timestamps associated with all commits, in order, of application 
(Jenkins seems happy to deal with this part), so that the files do keep 
relatively sane from a build perspective. Personally, I am relatively happy 
with this solution, even if it adds a huge amount of time to the build - 
generally more than the build itself - so that timestamps are "sane". Doing it 
for straight clones does not seem worth it, because timestamps don't appear to 
matter, policy wise, unless official builds are being done. It may be worth 
considering that in the discussion. 

My comments are just based on a production perspective, rather than 
development, so I ask forgiveness for any red-herrings that may be involved.

Cheers,
Randall

-- Brief whoami: NonStop developer since approximately 
UNIX(421664400)/NonStop(2112884442)
-- In my real life, I talk too much.



--
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: feature request: git svn dommit --preserve-timestamps

[Eric Wong]

Peter Münster  wrote:
> On Tue, Jun 07 2016, Eric Wong wrote:
> > Peter Münster  wrote:
> >> It would be nice, if timestamps could be preserved when rewriting the
> >> git-log.
> >
> > Unfortunately, last I checked (a long time ago!), explicitly
> > setting revprops might require SVN administrators to enable the
> > feature for the repo.
> 
> Not the svn-log, only the git-log.

The git log after dcommit is tied to the SVN log,
so git-svn can only reflect changes which appear in SVN.

Sidenote: The convention is reply-to-all on lists like
this one which do not require subscription to post.
It prevents the list from being a single-point-of-failure
or censorship.

> > It's been a while and I'm not up-to-date with the latest SVN.
> > Maybe there's a newer/easier way you could give us details about :)
> 
> No, sorry. I don't care about the svn-log.

Unfortunately, you would have to care about svn log as long as
SVN exists in your workflow and you need to interact with SVN
users.

git svn tries hard to work transparently and as close to the
behavior of the upstream SVN repo as possible.
--
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 00/27] nd/shallow-deepen updates

[Eric Sunshine]

On Fri, Jun 10, 2016 at 8:26 AM, Nguyễn Thái Ngọc Duy  wrote:
> This contains cleanups after Eric's comments (all good points,
> thanks!). Changed patches have Junio's s-o-b line removed, so it's
> easy to see which is changed and which is not. 09/27 is a new one,
> split out of 10/27.

Thanks, the interdiff looks sensible and I think it covers all the
points raised by my review of the previous round.

I agree with Junio that moving the sigchain_pop() into the error
handling code-path, if possible, would be a nice improvement.
--
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: [PATCHv2] submodule--helper: initial clone learns retry logic

[Stefan Beller]

On Thu, Jun 9, 2016 at 5:35 PM, Stefan Beller  wrote:
> Each submodule that is attempted to be cloned, will be retried once in
> case of failure after all other submodules were cloned. This helps to
> mitigate ephemeral server failures and increases chances of a reliable
> clone of a repo with hundreds of submodules immensely.
>
> Signed-off-by: Stefan Beller 
> ---
>
>  This patch doesn't do pointer forbidden magic, also we don't abuse the
>  prio queue.
>
>  The API for running parallel commands isn't quite designed for this
>  though as we need to pass around an int instead of a pointer, so we wrap
>  the int into a int*, which will create a xmalloc/free for each submodule.
>
>  This replaces the patch [1/2] in the series
>  "[PATCH 0/2] Dealing with a lot of submodules"
>
>  Thanks,
>  Stefan
>
>  builtin/submodule--helper.c | 66 
> -
>  1 file changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c7deb55..6424b40 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -590,10 +590,14 @@ struct submodule_update_clone {
>
> /* If we want to stop as fast as possible and return an error */
> unsigned quickstop : 1;
> +
> +   /* failed clones to be retried again */
> +   const struct cache_entry **failed_clones;
> +   int failed_clones_nr, failed_clones_alloc;
>  };
>  #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
> SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
> -   STRING_LIST_INIT_DUP, 0}
> +   STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
>
>
>  static void next_submodule_warn_missing(struct submodule_update_clone *suc,
> @@ -718,23 +722,47 @@ cleanup:
>  static int update_clone_get_next_task(struct child_process *child,
>   struct strbuf *err,
>   void *suc_cb,
> - void **void_task_cb)
> + void **idx_task_cb)
>  {
> struct submodule_update_clone *suc = suc_cb;
> +   const struct cache_entry *ce;
> +   int index;
>
> for (; suc->current < suc->list.nr; suc->current++) {
> -   const struct cache_entry *ce = 
> suc->list.entries[suc->current];
> +   ce = suc->list.entries[suc->current];
> if (prepare_to_clone_next_submodule(ce, child, suc, err)) {
> +   int *p = xmalloc(sizeof(*p));
> +   *p = suc->current;
> +   *idx_task_cb = p;
> suc->current++;
> return 1;
> }
> }
> +
> +   /*
> +* The loop above tried cloning each submodule once, now try the
> +* stragglers again, which we can imagine as an extension of the
> +* entry list.
> +*/
> +   index = suc->current - suc->list.nr;
> +   if (index < suc->failed_clones_nr) {
> +   int *p;
> +   ce = suc->failed_clones[index];
> +   if (!prepare_to_clone_next_submodule(ce, child, suc, err))
> +   die("BUG: ce was a submodule before?");
> +   p = xmalloc(sizeof(*p));
> +   *p = suc->current;
> +   *idx_task_cb = p;
> +   suc->current ++;
> +   return 1;
> +   }
> +
> return 0;
>  }
>
>  static int update_clone_start_failure(struct strbuf *err,
>   void *suc_cb,
> - void *void_task_cb)
> + void *idx_task_cb)
>  {

Here we would leak the pointer, but as we quickstop soon, this may not
be a problem.

> struct submodule_update_clone *suc = suc_cb;
> suc->quickstop = 1;
> @@ -744,15 +772,39 @@ static int update_clone_start_failure(struct strbuf 
> *err,
>  static int update_clone_task_finished(int result,
>   struct strbuf *err,
>   void *suc_cb,
> - void *void_task_cb)
> + void *idx_task_cb)
>  {
> +   const struct cache_entry *ce;
> struct submodule_update_clone *suc = suc_cb;
>
> +   int *idxP = *(int**)idx_task_cb;
> +   int idx = *idxP;
> +   free(idxP);
> +
> if (!result)
> return 0;
>
> -   suc->quickstop = 1;
> -   return 1;
> +   if (idx < suc->list.nr) {
> +   ce  = suc->list.entries[idx];
> +   strbuf_addf(err, _("Failed to clone '%s'. Retry scheduled"),
> +   ce->name);
> +   strbuf_addch(err, '\n');
> +   ALLOC_GROW(suc->failed_clones,
> +  suc->failed_clones_nr + 1,
> + 

[PATCH 1/3] submodule update: add `--init-default-path` switch

[Stefan Beller]

The new switch `--init-default-path` initializes the submodules which are
configured in `submodule.defaultUpdatePath` instead of those given as
command line arguments before updating. In the first implementation this
is made incompatible with further command line arguments as it is
unclear what the user means by

git submodule update --init --init-default-path 

This new switch allows to record more complex patterns as it saves
retyping them whenever you invoke update.

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt|  5 
 Documentation/git-submodule.txt | 13 --
 git-submodule.sh| 23 ++
 t/t7400-submodule-basic.sh  | 53 +
 4 files changed, 88 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 59d7046..1bc50af 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2735,6 +2735,11 @@ submodule.fetchJobs::
in parallel. A value of 0 will give some reasonable default.
If unset, it defaults to 1.
 
+submodule.defaultUpdatePath::
+   Specifies a set of submodules to initialize when calling
+   `git submodule --init-default-group` by using the pathspec
+   syntax.
+
 tag.sort::
This variable controls the sort ordering of tags when displayed by
linkgit:git-tag[1]. Without the "--sort=" option provided, the
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 13adebf..66f1de0 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -13,8 +13,8 @@ SYNOPSIS
  [--reference ] [--depth ] [--]  
[]
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
 'git submodule' [--quiet] init [--] [...]
-'git submodule' [--quiet] deinit [-f|--force] [--] ...
-'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
+'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
+'git submodule' [--quiet] update [--init[-default-path]] [--remote] 
[-N|--no-fetch]
  [-f|--force] [--rebase|--merge] [--reference ]
  [--depth ] [--recursive] [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
@@ -190,6 +190,10 @@ If the submodule is not yet initialized, and you just want 
to use the
 setting as stored in .gitmodules, you can automatically initialize the
 submodule with the `--init` option.
 
+You can configure a set of submodules using pathspec syntax in
+submodule.defaultUpdatePath you can use `--init-default-path` to initialize
+those before updating.
+
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and update any nested submodules within.
 --
@@ -353,6 +357,11 @@ the submodule itself.
Initialize all submodules for which "git submodule init" has not been
called so far before updating.
 
+--init-default-path::
+   This option is only valid for the update command.
+   Initialize all submodules configured in "`submodule.defaultUpdatePath`"
+   that have not been updated before.
+
 --name::
This option is only valid for the add command. It sets the submodule's
name to the given string instead of defaulting to its path. The name
diff --git a/git-submodule.sh b/git-submodule.sh
index 07290d0..22984b5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -8,8 +8,8 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
 USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference 
] [--]  []
or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]
or: $dashless [--quiet] init [--] [...]
-   or: $dashless [--quiet] deinit [-f|--force] [--] ...
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--reference ] 
[--recursive] [--] [...]
+   or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)
+   or: $dashless [--quiet] update [--init[-default-path]] [--remote] 
[-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference 
] [--recursive] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive] 
or: $dashless [--quiet] sync [--recursive] [--] [...]"
@@ -624,7 +624,12 @@ cmd_update()
GIT_QUIET=1
;;
-i|--init)
-   init=1
+   test -z $init || test $init = by_args || die "$(gettext 
"Only one of --init or --init-default-path may be used.")"
+   init=by_args
+   ;;
+   --init-default-path)
+   test -z $init || test $init = by_config || die 
"$(gettext "Only one of --init or --init-default-path may be used.")"
+   init=by_config
;;

[PATCH 3/3] completion: clone can recurse into submodules

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 contrib/completion/git-completion.bash | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index e3918c8..3d4d24a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1092,6 +1092,8 @@ _git_clone ()
--depth
--single-branch
--branch
+   --recursive
+   --init-submodule
"
return
;;
-- 
2.8.2.141.g4e46f88

--
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 2/3] clone: add --init-submodule= switch

[Stefan Beller]

The new switch passes the pathspec to `git submodule update --init`
which is called after the actual clone is done.

Additionally this configures the submodule.defaultUpdatePath to
be the given pathspec, such that any future invocation of
`git submodule update --init-default-paths` will keep up
with the pathspec.

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt | 23 +
 builtin/clone.c | 36 ++--
 t/t7400-submodule-basic.sh  | 81 +
 3 files changed, 131 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 1b15cd7..be05a87 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,8 @@ SYNOPSIS
  [--dissociate] [--separate-git-dir ]
  [--depth ] [--[no-]single-branch]
  [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
- [--jobs ] [--]  []
+ [--init-submodule ] [--jobs ] [--]
+  []
 
 DESCRIPTION
 ---
@@ -207,12 +208,20 @@ objects from the source repository into a pack in the 
cloned repository.
 
 --recursive::
 --recurse-submodules::
-   After the clone is created, initialize all submodules within,
-   using their default settings. This is equivalent to running
-   `git submodule update --init --recursive` immediately after
-   the clone is finished. This option is ignored if the cloned
-   repository does not have a worktree/checkout (i.e. if any of
-   `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+   After the clone is created, initialize and clone all submodules
+   within, using their default settings. This is equivalent to
+   running `git submodule update --recursive --init `
+   immediately after the clone is finished. This option is ignored
+   if the cloned repository does not have a worktree/checkout (i.e.
+   if any of `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+
+--init-submodule::
+   After the clone is created, initialize and clone specified
+   submodules within, using their default settings. It is possible
+   to give multiple specifications by giving this argument multiple
+   times. This is equivalent to configure `submodule.defaultUpdateGroup`
+   and then running `git submodule update --init-default-path`
+   immediately after the clone is finished.
 
 --[no-]shallow-submodules::
All submodules which are cloned will be shallow with a depth of 1.
diff --git a/builtin/clone.c b/builtin/clone.c
index 5f867e6..368014e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -53,6 +53,16 @@ static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
 static int max_jobs = -1;
+static struct string_list init_submodules;
+
+static int init_submodules_cb(const struct option *opt, const char *arg, int 
unset)
+{
+   if (unset)
+   return -1;
+
+   string_list_append((struct string_list *)opt->value, arg);
+   return 0;
+}
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(_verbosity),
@@ -103,6 +113,9 @@ static struct option builtin_clone_options[] = {
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", , N_("use IPv6 addresses only"),
TRANSPORT_FAMILY_IPV6),
+   OPT_CALLBACK(0, "init-submodule", _submodules, N_(""),
+   N_("clone specific submodules. Pass multiple times for 
complex pathspecs"),
+   init_submodules_cb),
OPT_END()
 };
 
@@ -734,14 +747,22 @@ static int checkout(void)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
   sha1_to_hex(sha1), "1", NULL);
 
-   if (!err && option_recursive) {
+   if (!err && (option_recursive || init_submodules.nr > 0)) {
struct argv_array args = ARGV_ARRAY_INIT;
-   argv_array_pushl(, "submodule", "update", "--init", 
"--recursive", NULL);
+   argv_array_pushl(, "submodule", "update", NULL);
+
+   if (init_submodules.nr > 0)
+   argv_array_pushf(, "--init-default-path");
+   else
+   argv_array_pushf(, "--init");
 
if (option_shallow_submodules == 1
|| (option_shallow_submodules == -1 && option_depth))
argv_array_push(, "--depth=1");
 
+   if (option_recursive)
+   argv_array_pushf(, "--recursive");
+
if (max_jobs != -1)
argv_array_pushf(, "--jobs=%d", max_jobs);
 
@@ -883,6 +904,17 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
option_no_checkout = 1;
}
 
+   if (init_submodules.nr > 0) {
+   struct string_list_item *item;
+   

[PATCHv4 0/3] Persistent submodule pathspec specification

[Stefan Beller]

This is a reroll of origin/sb/submodule-default-paths.

It is based on top of the current origin/sb/pathspec-label (d8e47e7d5c62e,
2016-06-02 pathspec: disable preload-index when attribute pathspec magic is in 
use)
which got merged with the current origin/sb/clone-shallow-passthru (d22eb0447,
clone: add `--shallow-submodules` flag, 2016-04-25)

As the merge itself produced merge conflicts, you can find my version of
resolving the merge at [1].

After the resolved conflict, this reroll
 * fixes 2 typos:
s/Pass ultiple/Pass multiple/ in builtin/clone.c and
s/submodulespec/pathspec/ in Documentation/git-clone.txt
 * adds another patch on top for bash completion of clone to include
--recurse and --init-submodule

Thanks,
Stefan

[1] https://github.com/stefanbeller/git/tree/submodule-default-paths

Stefan Beller (3):
  submodule update: add `--init-default-path` switch
  clone: add --init-submodule= switch
  completion: clone can recurse into submodules

 Documentation/config.txt   |   5 ++
 Documentation/git-clone.txt|  23 --
 Documentation/git-submodule.txt|  13 +++-
 builtin/clone.c|  36 -
 contrib/completion/git-completion.bash |   2 +
 git-submodule.sh   |  23 +-
 t/t7400-submodule-basic.sh | 134 +
 7 files changed, 221 insertions(+), 15 deletions(-)

-- 
2.8.2.141.g4e46f88

--
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] Fix spelling of "occurred"

[Junio C Hamano]

Peter Colberg  writes:

> On Fri, Jun 10, 2016 at 02:15:58PM -0700, Junio C Hamano wrote:
>> For this one, I am tempted to say that it may be better to remove
>> the verb altogether, which would lead to a more concise error
>> message.
>
> Thanks, I will send [PATCH v3] when the next release cycle begins.

There may not be a need for resend, though.  I already split them
into two and queued to my tree (two patches, pc/occurred topic
ending at 3a39f61e0).

Thanks.
--
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] Fix spelling of "occurred"

[Peter Colberg]

On Fri, Jun 10, 2016 at 02:15:58PM -0700, Junio C Hamano wrote:
> For this one, I am tempted to say that it may be better to remove
> the verb altogether, which would lead to a more concise error
> message.

Thanks, I will send [PATCH v3] when the next release cycle begins.

Peter
--
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 v6 41/44] am: use be_silent in 'struct apply_state' to shut up applying patches

[Junio C Hamano]

The update in 33/44 to make am call into apply that is not ready to
be called (e.g. the caller needs the dup(2) dance with /dev/null to
be silent) gets finally corrected with this step, which makes the
progress of the topic somewhat ugly and reviewing it a bit harder
than necessary.  As it stands, the last several patches in the
series smells more like "oops, we realize these things were not done
properly the first time, and here are the follow-up patches to fix
them up".

I wonder if it is a good idea to delay integrating the apply
machinery into "am" until it is ready?
--
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] diff: disable compaction heuristic for now

[Junio C Hamano]

Jeff King  writes:

> Looks good.
>
> I think your calendar calls for release 2.9.0 on Monday. Are you going
> to bump the schedule for this? I don't think it's very high risk, and
> wouldn't need to.

I didn't plan to.
--
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] Fix spelling of "occurred"

[Junio C Hamano]

Peter Colberg  writes:

> Signed-off-by: Peter Colberg 
> ---
>  config.c | 2 +-
>  refs.h   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index f51c56b..d7ce34b 100644
> --- a/config.c
> +++ b/config.c
> @@ -1281,7 +1281,7 @@ static void git_config_raw(config_fn_t fn, void *data)
>* something went really wrong and we should stop
>* immediately.
>*/
> - die(_("unknown error occured while reading the configuration 
> files"));
> + die(_("unknown error occurred while reading the configuration 
> files"));
>  }

For this one, I am tempted to say that it may be better to remove
the verb altogether, which would lead to a more concise error
message.

In any case, we need to postpone this until the next cycle, as it is
too late to change message strings now, as it would leave no time
for i18n/l10n team to adjust their work.

The other patch to the comment could be taken now if we wanted to,
though.

Thanks.

>  static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
> diff --git a/refs.h b/refs.h
> index 9230d47..56089d5 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -345,7 +345,7 @@ struct ref_transaction *ref_transaction_begin(struct 
> strbuf *err);
>   * msg -- a message describing the change (for the reflog).
>   *
>   * err -- a strbuf for receiving a description of any error that
> - * might have occured.
> + * might have occurred.
>   *
>   * The functions make internal copies of refname and msg, so the
>   * caller retains ownership of these parameters.
--
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] diff: disable compaction heuristic for now

[Jeff King]

On Fri, Jun 10, 2016 at 01:55:01PM -0700, Junio C Hamano wrote:

> This is for 'master' when the topic is merged (will keep it in stash
> for now).
> 
>  Documentation/RelNotes/2.9.0.txt | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/RelNotes/2.9.0.txt 
> b/Documentation/RelNotes/2.9.0.txt
> index 927cb9b..79b46e1 100644
> --- a/Documentation/RelNotes/2.9.0.txt
> +++ b/Documentation/RelNotes/2.9.0.txt
> @@ -118,9 +118,11 @@ UI, Workflows & Features
>   * HTTP transport clients learned to throw extra HTTP headers at the
> server, specified via http.extraHeader configuration variable.
>  
> - * Patch output from "git diff" and friends has been tweaked to be
> -   more readable by using a blank line as a strong hint that the
> -   contents before and after it belong to logically separate units.
> + * The "--compaction-heuristic" option to "git diff" family of
> +   commands enables a heuristic to make the patch output more readable
> +   by using a blank line as a strong hint that the contents before and
> +   after it belong to logically separate units.  It is still
> +   experimental.
>  
>   * A new configuration variable core.hooksPath allows customizing
> where the hook directory is.

Looks good.

I think your calendar calls for release 2.9.0 on Monday. Are you going
to bump the schedule for this? I don't think it's very high risk, and
wouldn't need to.

-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

Re: [PATCH v6 43/44] builtin/apply: add a cli option for be_silent

[René Scharfe]

Am 10.06.2016 um 22:11 schrieb Christian Couder:

Let's make it possible to request a silent operation on the
command line.

Signed-off-by: Christian Couder 
---
  builtin/apply.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index ddd61de..93744f8 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -74,6 +74,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "allow-overlap", _overlap,
N_("allow overlapping hunks")),
OPT__VERBOSE(_verbosely, N_("be verbose")),
+   OPT_BOOL(0, "silent", _silent,
+   N_("do not print any output")),
OPT_BIT(0, "inaccurate-eof", ,
N_("tolerate incorrectly detected missing new-line at the 
end of file"),
APPLY_OPT_INACCURATE_EOF),


Why not -q/--quiet as for most other commands?

Furthermore, you could use OPT__VERBOSITY, which causes -v and -q to 
update the same variable variable and thus lets parseopt handle their 
interaction.  Perhaps verbosity == 1 could mean verbose, 0 normal, -1 no 
infos, -2 no warnings and -3 no errors?


And if you add the ability to silence the apply functions before using 
them you don't have to export and unexport dup_devnull().


René

--
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] diff: disable compaction heuristic for now

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
> > We probably want a patch to the release notes to note that it's not on
> > by default. And we may want to advertise the experimental knob so
> > that people actually try it (otherwise we won't get feedback from the
> > masses).
>
> OK, I think that is sensible.  The interdiff is not a strict
> reversion of 77085a61 (diff: undocument the compaction heuristic
> knobs for experimentation, 2016-05-02) but stresses that the
> feature is off by default and is experimental.

This is for 'master' when the topic is merged (will keep it in stash
for now).

 Documentation/RelNotes/2.9.0.txt | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/RelNotes/2.9.0.txt b/Documentation/RelNotes/2.9.0.txt
index 927cb9b..79b46e1 100644
--- a/Documentation/RelNotes/2.9.0.txt
+++ b/Documentation/RelNotes/2.9.0.txt
@@ -118,9 +118,11 @@ UI, Workflows & Features
  * HTTP transport clients learned to throw extra HTTP headers at the
server, specified via http.extraHeader configuration variable.
 
- * Patch output from "git diff" and friends has been tweaked to be
-   more readable by using a blank line as a strong hint that the
-   contents before and after it belong to logically separate units.
+ * The "--compaction-heuristic" option to "git diff" family of
+   commands enables a heuristic to make the patch output more readable
+   by using a blank line as a strong hint that the contents before and
+   after it belong to logically separate units.  It is still
+   experimental.
 
  * A new configuration variable core.hooksPath allows customizing
where the hook directory is.
--
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] diff: disable compaction heuristic for now

[Jeff King]

On Fri, Jun 10, 2016 at 01:48:58PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > We probably want a patch to the release notes to note that it's not on
> > by default. And we may want to advertise the experimental knob so
> > that people actually try it (otherwise we won't get feedback from the
> > masses).
> 
> OK, I think that is sensible.  The interdiff is not a strict
> reversion of 77085a61 (diff: undocument the compaction heuristic
> knobs for experimentation, 2016-05-02) but stresses that the
> feature is off by default and is experimental.

Looks good to me. Do we want to include in "experimental" that the
option and command-line flag might go away in the future? I think
probably not. I do not mind supporting this forever (it _has_ shown its
usefulness, so I don't think it is any worse to maintain going forward
than things like --patience, which people can tweak if their diff looks
uglier than they would like).

-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 v2] diff: disable compaction heuristic for now

[Junio C Hamano]

http://lkml.kernel.org/g/20160610075043.ga13...@sigill.intra.peff.net
reports that a change to add a new "function" with common ending
with the existing one at the end of the file is shown like this:

def foo
  do_foo_stuff()

   +  common_ending()
   +end
   +
   +def bar
   +  do_bar_stuff()
   +
  common_ending()
end

when the new heuristic is in use.  In reality, the change is to add
the blank line before "def bar" and everything below, which is what
the code without the new heuristic shows.

Disable the heuristics by default, and resurrect the documentation
for the option and the configuration variables, while clearly
marking the feature as still experimental.

Signed-off-by: Junio C Hamano 
---

Jeff King  writes:

> We probably want a patch to the release notes to note that it's not on
> by default. And we may want to advertise the experimental knob so
> that people actually try it (otherwise we won't get feedback from the
> masses).

OK, I think that is sensible.  The interdiff is not a strict
reversion of 77085a61 (diff: undocument the compaction heuristic
knobs for experimentation, 2016-05-02) but stresses that the
feature is off by default and is experimental.

 Documentation/diff-config.txt  | 5 +
 Documentation/diff-options.txt | 7 +++
 diff.c | 2 +-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 6eaa452..6fb70c5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -166,6 +166,11 @@ diff.tool::
 
 include::mergetools-diff.txt[]
 
+diff.compactionHeuristic::
+   Set this option to `true` to enable an experimental heuristic that
+   shifts the hunk boundary in an attempt to make the resulting
+   patch easier to read.
+
 diff.algorithm::
Choose a diff algorithm.  The variants are as follows:
 +
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 3ad6404..9baf1ad 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -63,6 +63,13 @@ ifndef::git-format-patch[]
Synonym for `-p --raw`.
 endif::git-format-patch[]
 
+--compaction-heuristic::
+--no-compaction-heuristic::
+   These are to help debugging and tuning an experimental
+   heuristic (which is off by default) that shifts the hunk
+   boundary in an attempt to make the resulting patch easier
+   to read.
+
 --minimal::
Spend extra time to make sure the smallest possible
diff is produced.
diff --git a/diff.c b/diff.c
index 05ca3ce..9116d9d 100644
--- a/diff.c
+++ b/diff.c
@@ -25,7 +25,7 @@
 #endif
 
 static int diff_detect_rename_default;
-static int diff_compaction_heuristic = 1;
+static int diff_compaction_heuristic; /* experimental */
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
-- 
2.9.0-rc2-285-ge226c12

--
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 10/27] upload-pack: move rev-list code out of check_non_tip()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> -static void check_non_tip(void)
> +static int check_unreachable(struct object_array *src)
>  {
>...
>   /* All the non-tip ones are ancestors of what we advertised */
> - return;
> + return 1;
>  
>  error:
>   if (cmd.in >= 0)
>   close(cmd.in);
>   if (cmd.out >= 0)
>   close(cmd.out);
> + return 0;
> +}

"zero is bad, one is good" is OK as a helper function that is local.
The convention needs to be documented as a comment before the
function.

Perhaps if you avoid using "check_", which does not hint the return
value, both the function and the caller would become easier to read.

How about calling it has_unreachable() and make the caller say

if (!has_unreachable(_obj))
/* all requested tips are reachable from what we advertised */
return;

instead?

> +static void check_non_tip(void)
> +{
> + int i;
> +
> + /*
> +  * In the normal in-process case without
> +  * uploadpack.allowReachableSHA1InWant,
> +  * non-tip requests can never happen.
> +  */
> + if (!stateless_rpc && !(allow_unadvertised_object_request & 
> ALLOW_REACHABLE_SHA1))
> + goto error;
> + if (check_unreachable(_obj))
> + /* All the non-tip ones are ancestors of what we advertised */
> + return;
> +
> +error:
>   /* Pick one of them (we know there at least is one) */
>   for (i = 0; i < want_obj.nr; i++) {
> - o = want_obj.objects[i].item;
> + struct object *o = want_obj.objects[i].item;
>   if (!is_our_ref(o))
>   die("git upload-pack: not our ref %s",
>   oid_to_hex(>oid));
--
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 00/94] libify apply and use lib in am

[Christian Couder]

On Fri, Jun 10, 2016 at 7:04 PM, Johannes Sixt  wrote:
> Am 10.06.2016 um 13:11 schrieb Johannes Schindelin:
>>
>> Not really. The reply (which I had not quite connected with my mail
>> because they were over a week apart) says this:
>>
>>> I fixed this by moving the "close(fd)" call just after the
>>> "apply_patch()" call.
>
> This bug in v1 was discovered by the test suite and fixed in v2.
>
>> and this:
>>
>>> I will have another look at the 2 other places where there are
>>> open()/close() or fopen()/fclose() calls.
>>
>> but nothing about a careful, systematic investigation of all error code
>> paths. As a consequence, I fully expect to encounter test failures as soon
>> as I test your patch series again, simply because resources are still in
>> use when they should no longer be used. In other words, my expectations
>> are now lower than they have been before, my concerns are not at all
>> addressed.
>
> Do you trust the test suite to some degree? It passes after the above bug
> was fixed in v2. In addition, haven't found any problems so far during daily
> use.
>
>>> This is the newest iteration:
>>>
>>> https://github.com/chriscool/git/commits/libify-apply-use-in-am65
>>
>> And that cute 65 in the name is the revision.
>
> Yeah, that number is painful. I would appreciate an unversioned branch name,
> too.

Ok, you can use
https://github.com/chriscool/git/commits/libify-apply-use-in-am then.

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: Re* [BUG-ish] diff compaction heuristic false positive

[Jeff King]

On Fri, Jun 10, 2016 at 11:13:10AM -0700, Junio C Hamano wrote:

> Jacob Keller  writes:
> 
> > I think we could use the indentation trick and it might help in this
> > case. I agree, let's disable this for this cycle and experiment in the
> > next one. Good catch, Peff.
> >
> > As others have said you will always be able to produce counter
> > examples, that's the nature of heuristics. The idea is to see if we
> > can come up with something simple that mostly improves the output,
> > even if sometimes it might have a negative impact on the outputs. But
> > I think we should avoid changing behavior unless it's mostly an
> > improvement.
> 
> OK, let's do this then for the upcoming release for now.  I am
> tempted to flip it back on after the release in 'next', so that
> developers would be exposed to the heuristic by default, though.

Thanks for the patch, and I agree flipping it back on in "next" is a
good idea.

It may be that I am making a fuss over nothing. As you say, we always
knew that it might have false positives. Mostly I was just surprised how
frequent they were in this example (I also wondered why the same pattern
did not trigger in the C code we looked at).

> -- >8 --
> Subject: [PATCH] diff: disable compaction heuristic for now

Looks good.

We probably want a patch to the release notes to note that it's not on
by default. And we may want to advertise the experimental knob so
that people actually try it (otherwise we won't get feedback from the
masses).

-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

Re: [PATCH 09/27] upload-pack: make check_non_tip() clean things up error

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Subject: Re: [PATCH 09/27] upload-pack: make check_non_tip() clean things up 
> error

"clean things up on error"?

> On error check_non_tip() will die and not closing file descriptors is no
> big deal. The next patch will split the majority of this function out
> for reuse in other cases, where die() may not be the only outcome. Same
> story for popping SIGPIPE out of the signal chain. So let's make sure we
> clean things up properly first.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 

Makes me wonder if you can push into sigchain before the first
appearance of "goto error" so that in the error handling codepath
you can do sigchain_pop(), without adding sigchain_pop() before all
the "goto error"?

>  upload-pack.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 60f2e5e..1e8b025 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -494,8 +494,10 @@ static void check_non_tip(void)
>   if (!is_our_ref(o))
>   continue;
>   memcpy(namebuf + 1, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
> - if (write_in_full(cmd.in, namebuf, 42) < 0)
> + if (write_in_full(cmd.in, namebuf, 42) < 0) {
> + sigchain_pop(SIGPIPE);
>   goto error;
> + }
>   }
>   namebuf[40] = '\n';
>   for (i = 0; i < want_obj.nr; i++) {
> @@ -503,10 +505,13 @@ static void check_non_tip(void)
>   if (is_our_ref(o))
>   continue;
>   memcpy(namebuf, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
> - if (write_in_full(cmd.in, namebuf, 41) < 0)
> + if (write_in_full(cmd.in, namebuf, 41) < 0) {
> + sigchain_pop(SIGPIPE);
>   goto error;
> + }
>   }
>   close(cmd.in);
> + cmd.in = -1;
>  
>   sigchain_pop(SIGPIPE);
>  
> @@ -518,6 +523,7 @@ static void check_non_tip(void)
>   if (i)
>   goto error;
>   close(cmd.out);
> + cmd.out = -1;
>  
>   /*
>* rev-list may have died by encountering a bad commit
> @@ -531,6 +537,11 @@ static void check_non_tip(void)
>   return;
>  
>  error:
> + if (cmd.in >= 0)
> + close(cmd.in);
> + if (cmd.out >= 0)
> + close(cmd.out);
> +
>   /* Pick one of them (we know there at least is one) */
>   for (i = 0; i < want_obj.nr; i++) {
>   o = want_obj.objects[i].item;
--
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 v6 20/44] builtin/apply: make add_conflicted_stages_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", add_conflicted_stages_file() should return -1
instead of calling die().

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 694c65b..0997384 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4213,7 +4213,7 @@ static void create_one_file(struct apply_state *state,
die_errno(_("unable to write file '%s' mode %o"), path, mode);
 }
 
-static void add_conflicted_stages_file(struct apply_state *state,
+static int add_conflicted_stages_file(struct apply_state *state,
   struct patch *patch)
 {
int stage, namelen;
@@ -4221,7 +4221,7 @@ static void add_conflicted_stages_file(struct apply_state 
*state,
struct cache_entry *ce;
 
if (!state->update_index)
-   return;
+   return 0;
namelen = strlen(patch->new_name);
ce_size = cache_entry_size(namelen);
mode = patch->new_mode ? patch->new_mode : (S_IFREG | 0644);
@@ -4236,9 +4236,14 @@ static void add_conflicted_stages_file(struct 
apply_state *state,
ce->ce_flags = create_ce_flags(stage);
ce->ce_namelen = namelen;
hashcpy(ce->sha1, patch->threeway_stage[stage - 1].hash);
-   if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0)
-   die(_("unable to add cache entry for %s"), 
patch->new_name);
+   if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) {
+   free(ce);
+   return error(_("unable to add cache entry for %s"),
+patch->new_name);
+   }
}
+
+   return 0;
 }
 
 static void create_file(struct apply_state *state, struct patch *patch)
@@ -4252,9 +4257,10 @@ static void create_file(struct apply_state *state, 
struct patch *patch)
mode = S_IFREG | 0644;
create_one_file(state, path, mode, buf, size);
 
-   if (patch->conflicted_threeway)
-   add_conflicted_stages_file(state, patch);
-   else
+   if (patch->conflicted_threeway) {
+   if (add_conflicted_stages_file(state, patch))
+   exit(1);
+   } else
add_index_file(state, path, mode, buf, size);
 }
 
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 08/44] builtin/apply: make parse_ignorewhitespace_option() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", parse_ignorewhitespace_option() should return
-1 instead of calling die().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index e56e754..e2f970d 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -57,20 +57,20 @@ static int parse_whitespace_option(struct apply_state 
*state, const char *option
return error(_("unrecognized whitespace option '%s'"), option);
 }
 
-static void parse_ignorewhitespace_option(struct apply_state *state,
- const char *option)
+static int parse_ignorewhitespace_option(struct apply_state *state,
+const char *option)
 {
if (!option || !strcmp(option, "no") ||
!strcmp(option, "false") || !strcmp(option, "never") ||
!strcmp(option, "none")) {
state->ws_ignore_action = ignore_ws_none;
-   return;
+   return 0;
}
if (!strcmp(option, "change")) {
state->ws_ignore_action = ignore_ws_change;
-   return;
+   return 0;
}
-   die(_("unrecognized whitespace ignore option '%s'"), option);
+   return error(_("unrecognized whitespace ignore option '%s'"), option);
 }
 
 static void set_default_whitespace_mode(struct apply_state *state)
@@ -4616,8 +4616,8 @@ static void init_apply_state(struct apply_state *state,
git_apply_config();
if (apply_default_whitespace && parse_whitespace_option(state, 
apply_default_whitespace))
exit(1);
-   if (apply_default_ignorewhitespace)
-   parse_ignorewhitespace_option(state, 
apply_default_ignorewhitespace);
+   if (apply_default_ignorewhitespace && 
parse_ignorewhitespace_option(state, apply_default_ignorewhitespace))
+   exit(1);
 }
 
 static void clear_apply_state(struct apply_state *state)
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 36/44] apply: make 'be_silent' incompatible with 'apply_verbosely'

[Christian Couder]

It should be an error to have both be_silent and apply_verbosely set,
so let's check that in check_apply_state().

And by the way let's not automatically set apply_verbosely when
be_silent is set.

Signed-off-by: Christian Couder 
---
 apply.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index dbb2515..dd9b301 100644
--- a/apply.c
+++ b/apply.c
@@ -122,8 +122,11 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
return error(_("--3way outside a repository"));
state->check_index = 1;
}
-   if (state->apply_with_reject)
-   state->apply = state->apply_verbosely = 1;
+   if (state->apply_with_reject) {
+   state->apply = 1;
+   if (!state->be_silent)
+   state->apply_verbosely = 1;
+   }
if (!force_apply && (state->diffstat || state->numstat || 
state->summary || state->check || state->fake_ancestor))
state->apply = 0;
if (state->check_index && is_not_gitdir)
@@ -135,6 +138,8 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
}
if (state->check_index)
state->unsafe_paths = 0;
+   if (state->be_silent && state->apply_verbosely)
+   return error(_("incompatible internal 'be_silent' and 
'apply_verbosely' flags"));
if (!state->lock_file)
return error("BUG: state->lock_file should not be NULL");
 
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 15/44] builtin/apply: make gitdiff_*() return 1 at end of header

[Christian Couder]

The gitdiff_*() functions that are called as p->fn() in parse_git_header()
should return 1 instead of -1 in case of end of header or unrecognized
input, as these are not real errors. It just instructs the parser to break
out.

This makes it possible for gitdiff_*() functions to return -1 in case of a
real error. This will be done in a following patch.

Helped-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index eb98116..1142514 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -812,7 +812,7 @@ static int gitdiff_hdrend(struct apply_state *state,
  const char *line,
  struct patch *patch)
 {
-   return -1;
+   return 1;
 }
 
 /*
@@ -1016,7 +1016,7 @@ static int gitdiff_unrecognized(struct apply_state *state,
const char *line,
struct patch *patch)
 {
-   return -1;
+   return 1;
 }
 
 /*
@@ -1248,9 +1248,13 @@ static int parse_git_header(struct apply_state *state,
for (i = 0; i < ARRAY_SIZE(optable); i++) {
const struct opentry *p = optable + i;
int oplen = strlen(p->str);
+   int res;
if (len < oplen || memcmp(p->str, line, oplen))
continue;
-   if (p->fn(state, line + oplen, patch) < 0)
+   res = p->fn(state, line + oplen, patch);
+   if (res < 0)
+   return -1;
+   if (res > 0)
return offset;
break;
}
@@ -1429,6 +1433,8 @@ static int find_header(struct apply_state *state,
 */
if (!memcmp("diff --git ", line, 11)) {
int git_hdr_len = parse_git_header(state, line, len, 
size, patch);
+   if (git_hdr_len < 0)
+   return -1;
if (git_hdr_len <= len)
continue;
if (!patch->old_name && !patch->new_name) {
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 43/44] builtin/apply: add a cli option for be_silent

[Christian Couder]

Let's make it possible to request a silent operation on the
command line.

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index ddd61de..93744f8 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -74,6 +74,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "allow-overlap", _overlap,
N_("allow overlapping hunks")),
OPT__VERBOSE(_verbosely, N_("be verbose")),
+   OPT_BOOL(0, "silent", _silent,
+   N_("do not print any output")),
OPT_BIT(0, "inaccurate-eof", ,
N_("tolerate incorrectly detected missing new-line at 
the end of file"),
APPLY_OPT_INACCURATE_EOF),
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 33/44] builtin/am: use apply api in run_apply()

[Christian Couder]

This replaces run_apply() implementation with a new one that
uses the apply api that has been previously prepared in
apply.c and apply.h.

This shoud improve performance a lot in certain cases.

As the previous implementation was creating a new `git apply`
process to apply each patch, it could be slow on systems like
Windows where it is costly to create new processes.

Also the new `git apply` process had to read the index from
disk, and when the process was done the calling process
discarded its own index and read back from disk the new
index that had been created by the `git apply` process.

This could be very inefficient with big repositories that
have big index files, especially when the system decided
that it was a good idea to run the `git apply` processes on
a different processor core.

Also eliminating index reads enables further performance
improvements by using:

`git update-index --split-index`

For example here is a benchmark of a multi hundred commit
rebase on the Linux kernel on a Debian laptop with SSD:

command: git rebase --onto 1993b17 52bef0c 29dde7c

Vanilla "next" without split index:1m54.953s
Vanilla "next" with split index:   1m22.476s
This series on top of "next" without split index:  1m12.034s
This series on top of "next" with split index: 0m15.678s

(using branch "next" from mid April 2016.)

Benchmarked-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Christian Couder 
---
 builtin/am.c | 104 ---
 1 file changed, 86 insertions(+), 18 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3dfe70b..a16b06c 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -28,6 +28,7 @@
 #include "rerere.h"
 #include "prompt.h"
 #include "mailinfo.h"
+#include "apply.h"
 
 /**
  * Returns 1 if the file is empty or does not exist, 0 otherwise.
@@ -1521,39 +1522,106 @@ static int parse_mail_rebase(struct am_state *state, 
const char *mail)
  */
 static int run_apply(const struct am_state *state, const char *index_file)
 {
-   struct child_process cp = CHILD_PROCESS_INIT;
-
-   cp.git_cmd = 1;
-
-   if (index_file)
-   argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
index_file);
+   struct argv_array apply_paths = ARGV_ARRAY_INIT;
+   struct argv_array apply_opts = ARGV_ARRAY_INIT;
+   struct apply_state apply_state;
+   int save_stdout_fd, save_stderr_fd;
+   int res, opts_left;
+   char *save_index_file;
+   static struct lock_file lock_file;
+
+   struct option am_apply_options[] = {
+   { OPTION_CALLBACK, 0, "whitespace", _state, N_("action"),
+   N_("detect new or modified lines that have whitespace 
errors"),
+   0, apply_option_parse_whitespace },
+   { OPTION_CALLBACK, 0, "ignore-space-change", _state, NULL,
+   N_("ignore changes in whitespace when finding context"),
+   PARSE_OPT_NOARG, apply_option_parse_space_change },
+   { OPTION_CALLBACK, 0, "ignore-whitespace", _state, NULL,
+   N_("ignore changes in whitespace when finding context"),
+   PARSE_OPT_NOARG, apply_option_parse_space_change },
+   { OPTION_CALLBACK, 0, "directory", _state, N_("root"),
+   N_("prepend  to all filenames"),
+   0, apply_option_parse_directory },
+   { OPTION_CALLBACK, 0, "exclude", _state, N_("path"),
+   N_("don't apply changes matching the given path"),
+   0, apply_option_parse_exclude },
+   { OPTION_CALLBACK, 0, "include", _state, N_("path"),
+   N_("apply changes matching the given path"),
+   0, apply_option_parse_include },
+   OPT_INTEGER('C', NULL, _state.p_context,
+   N_("ensure at least  lines of context 
match")),
+   { OPTION_CALLBACK, 'p', NULL, _state, N_("num"),
+   N_("remove  leading slashes from traditional diff 
paths"),
+   0, apply_option_parse_p },
+   OPT_BOOL(0, "reject", _state.apply_with_reject,
+   N_("leave the rejected hunks in corresponding *.rej 
files")),
+   OPT_END()
+   };
 
/*
 * If we are allowed to fall back on 3-way merge, don't give false
 * errors during the initial attempt.
 */
+
if (state->threeway && !index_file) {
-   cp.no_stdout = 1;
-   cp.no_stderr = 1;
+   save_stdout_fd = dup(1);
+   dup_devnull(1);
+   save_stderr_fd = dup(2);
+   dup_devnull(2);
}
 
-   argv_array_push(, "apply");
+   if (index_file) {
+   save_index_file = get_index_file();
+   set_index_file((char 

[PATCH v6 37/44] apply: don't print on stdout when be_silent is set

[Christian Couder]

Signed-off-by: Christian Couder 
---
 apply.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/apply.c b/apply.c
index dd9b301..2529534 100644
--- a/apply.c
+++ b/apply.c
@@ -4679,13 +4679,13 @@ static int apply_patch(struct apply_state *state,
goto end;
}
 
-   if (state->diffstat)
+   if (state->diffstat && !state->be_silent)
stat_patch_list(state, list);
 
-   if (state->numstat)
+   if (state->numstat && !state->be_silent)
numstat_patch_list(state, list);
 
-   if (state->summary)
+   if (state->summary && !state->be_silent)
summary_patch_list(list);
 
 end:
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 32/44] environment: add set_index_file()

[Christian Couder]

Introduce set_index_file() to be able to temporarily change the index file.

It should be used like this:

/* Save current index file */
old_index_file = get_index_file();
set_index_file((char *)tmp_index_file);

/* Do stuff that will use tmp_index_file as the index file */
...

/* When finished reset the index file */
set_index_file(old_index_file);

Signed-off-by: Christian Couder 
---
 cache.h   |  1 +
 environment.c | 10 ++
 2 files changed, 11 insertions(+)

diff --git a/cache.h b/cache.h
index 81d4ac3..28fc0bf 100644
--- a/cache.h
+++ b/cache.h
@@ -461,6 +461,7 @@ extern int is_inside_work_tree(void);
 extern const char *get_git_dir(void);
 extern const char *get_git_common_dir(void);
 extern char *get_object_directory(void);
+extern void set_index_file(char *index_file);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
diff --git a/environment.c b/environment.c
index ca72464..7a53799 100644
--- a/environment.c
+++ b/environment.c
@@ -292,6 +292,16 @@ int odb_pack_keep(char *name, size_t namesz, const 
unsigned char *sha1)
return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
 }
 
+/*
+ * Temporarily change the index file.
+ * Please save the current index file using get_index_file() before changing
+ * the index file. And when finished, reset it to the saved value.
+ */
+void set_index_file(char *index_file)
+{
+   git_index_file = index_file;
+}
+
 char *get_index_file(void)
 {
if (!git_index_file)
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 19/44] builtin/apply: make remove_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", remove_file() should return -1 instead of
calling die().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index e74b068..694c65b 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4074,17 +4074,18 @@ static void patch_stats(struct apply_state *state, 
struct patch *patch)
}
 }
 
-static void remove_file(struct apply_state *state, struct patch *patch, int 
rmdir_empty)
+static int remove_file(struct apply_state *state, struct patch *patch, int 
rmdir_empty)
 {
if (state->update_index) {
if (remove_file_from_cache(patch->old_name) < 0)
-   die(_("unable to remove %s from index"), 
patch->old_name);
+   return error(_("unable to remove %s from index"), 
patch->old_name);
}
if (!state->cached) {
if (!remove_or_warn(patch->old_mode, patch->old_name) && 
rmdir_empty) {
remove_path(patch->old_name);
}
}
+   return 0;
 }
 
 static void add_index_file(struct apply_state *state,
@@ -4263,8 +4264,10 @@ static void write_out_one_result(struct apply_state 
*state,
 int phase)
 {
if (patch->is_delete > 0) {
-   if (phase == 0)
-   remove_file(state, patch, 1);
+   if (phase == 0) {
+   if (remove_file(state, patch, 1))
+   exit(1);
+   }
return;
}
if (patch->is_new > 0 || patch->is_copy) {
@@ -4276,8 +4279,10 @@ static void write_out_one_result(struct apply_state 
*state,
 * Rename or modification boils down to the same
 * thing: remove the old, write the new
 */
-   if (phase == 0)
-   remove_file(state, patch, patch->is_rename);
+   if (phase == 0) {
+   if (remove_file(state, patch, patch->is_rename))
+   exit(1);
+   }
if (phase == 1)
create_file(state, patch);
 }
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 44/44] apply: use error_errno() where possible

[Christian Couder]

Signed-off-by: Christian Couder 
---
 apply.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/apply.c b/apply.c
index ef49709..cd4cd01 100644
--- a/apply.c
+++ b/apply.c
@@ -3505,7 +3505,7 @@ static int load_current(struct apply_state *state,
ce = active_cache[pos];
if (lstat(name, )) {
if (errno != ENOENT)
-   return error(_("%s: %s"), name, strerror(errno));
+   return error_errno("%s", name);
if (checkout_target(_index, ce, ))
return -1;
}
@@ -3664,7 +3664,7 @@ static int check_preimage(struct apply_state *state,
} else if (!state->cached) {
stat_ret = lstat(old_name, st);
if (stat_ret && errno != ENOENT)
-   return error(_("%s: %s"), old_name, strerror(errno));
+   return error_errno("%s", old_name);
}
 
if (state->check_index && !previous) {
@@ -3686,7 +3686,7 @@ static int check_preimage(struct apply_state *state,
} else if (stat_ret < 0) {
if (patch->is_new < 0)
goto is_new;
-   return error(_("%s: %s"), old_name, strerror(errno));
+   return error_errno("%s", old_name);
}
 
if (!state->cached && !previous)
@@ -3745,7 +3745,7 @@ static int check_to_create(struct apply_state *state,
 
return EXISTS_IN_WORKTREE;
} else if ((errno != ENOENT) && (errno != ENOTDIR)) {
-   return error("%s: %s", new_name, strerror(errno));
+   return error_errno("%s", new_name);
}
return 0;
 }
@@ -4260,9 +4260,9 @@ static int add_index_file(struct apply_state *state,
if (!state->cached) {
if (lstat(path, ) < 0) {
free(ce);
-   return error(_("unable to stat newly "
-  "created file '%s': %s"),
-path, strerror(errno));
+   return error_errno(_("unable to stat newly "
+"created file '%s'"),
+  path);
}
fill_stat_cache_info(ce, );
}
@@ -4316,7 +4316,7 @@ static int try_create_file(const char *path, unsigned int 
mode, const char *buf,
strbuf_release();
 
if (close(fd) < 0 && !res)
-   return error(_("closing file '%s': %s"), path, strerror(errno));
+   return error_errno(_("closing file '%s'"), path);
 
return res ? -1 : 0;
 }
@@ -4386,8 +4386,8 @@ static int create_one_file(struct apply_state *state,
++nr;
}
}
-   return error(_("unable to write file '%s' mode %o: %s"),
-path, mode, strerror(errno));
+   return error_errno(_("unable to write file '%s' mode %o: %s"),
+  path, mode);
 }
 
 static int add_conflicted_stages_file(struct apply_state *state,
@@ -4514,7 +4514,7 @@ static int write_out_one_reject(struct apply_state 
*state, struct patch *patch)
 
rej = fopen(namebuf, "w");
if (!rej)
-   return error(_("cannot open %s: %s"), namebuf, strerror(errno));
+   return error_errno(_("cannot open %s"), namebuf);
 
/* Normal git tools never deal with .rej, so do not pretend
 * this is a git patch by saying --git or giving extended
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 09/44] builtin/apply: move init_apply_state() to apply.c

[Christian Couder]

To libify `git apply` functionality we must make init_apply_state()
usable outside "builtin/apply.c".

Let's do that by moving it into a new "apply.c".

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 Makefile|  1 +
 apply.c | 91 +
 apply.h | 10 +++
 builtin/apply.c | 88 ---
 4 files changed, 102 insertions(+), 88 deletions(-)
 create mode 100644 apply.c

diff --git a/Makefile b/Makefile
index 6b05249..6da1209 100644
--- a/Makefile
+++ b/Makefile
@@ -680,6 +680,7 @@ LIB_OBJS += abspath.o
 LIB_OBJS += advice.o
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
+LIB_OBJS += apply.o
 LIB_OBJS += archive.o
 LIB_OBJS += archive-tar.o
 LIB_OBJS += archive-zip.o
diff --git a/apply.c b/apply.c
new file mode 100644
index 000..1f31bb4
--- /dev/null
+++ b/apply.c
@@ -0,0 +1,91 @@
+#include "cache.h"
+#include "lockfile.h"
+#include "apply.h"
+
+static void git_apply_config(void)
+{
+   git_config_get_string_const("apply.whitespace", 
_default_whitespace);
+   git_config_get_string_const("apply.ignorewhitespace", 
_default_ignorewhitespace);
+   git_config(git_default_config, NULL);
+}
+
+int parse_whitespace_option(struct apply_state *state, const char *option)
+{
+   if (!option) {
+   state->ws_error_action = warn_on_ws_error;
+   return 0;
+   }
+   if (!strcmp(option, "warn")) {
+   state->ws_error_action = warn_on_ws_error;
+   return 0;
+   }
+   if (!strcmp(option, "nowarn")) {
+   state->ws_error_action = nowarn_ws_error;
+   return 0;
+   }
+   if (!strcmp(option, "error")) {
+   state->ws_error_action = die_on_ws_error;
+   return 0;
+   }
+   if (!strcmp(option, "error-all")) {
+   state->ws_error_action = die_on_ws_error;
+   state->squelch_whitespace_errors = 0;
+   return 0;
+   }
+   if (!strcmp(option, "strip") || !strcmp(option, "fix")) {
+   state->ws_error_action = correct_ws_error;
+   return 0;
+   }
+   return error(_("unrecognized whitespace option '%s'"), option);
+}
+
+int parse_ignorewhitespace_option(struct apply_state *state,
+ const char *option)
+{
+   if (!option || !strcmp(option, "no") ||
+   !strcmp(option, "false") || !strcmp(option, "never") ||
+   !strcmp(option, "none")) {
+   state->ws_ignore_action = ignore_ws_none;
+   return 0;
+   }
+   if (!strcmp(option, "change")) {
+   state->ws_ignore_action = ignore_ws_change;
+   return 0;
+   }
+   return error(_("unrecognized whitespace ignore option '%s'"), option);
+}
+
+void init_apply_state(struct apply_state *state,
+ const char *prefix,
+ struct lock_file *lock_file)
+{
+   memset(state, 0, sizeof(*state));
+   state->prefix = prefix;
+   state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
+   state->lock_file = lock_file;
+   state->newfd = -1;
+   state->apply = 1;
+   state->line_termination = '\n';
+   state->p_value = 1;
+   state->p_context = UINT_MAX;
+   state->squelch_whitespace_errors = 5;
+   state->ws_error_action = warn_on_ws_error;
+   state->ws_ignore_action = ignore_ws_none;
+   state->linenr = 1;
+   strbuf_init(>root, 0);
+
+   git_apply_config();
+   if (apply_default_whitespace && parse_whitespace_option(state, 
apply_default_whitespace))
+   exit(1);
+   if (apply_default_ignorewhitespace && 
parse_ignorewhitespace_option(state, apply_default_ignorewhitespace))
+   exit(1);
+}
+
+void clear_apply_state(struct apply_state *state)
+{
+   string_list_clear(>limit_by_name, 0);
+   string_list_clear(>symlink_changes, 0);
+   strbuf_release(>root);
+
+   /* >fn_table is cleared at the end of apply_patch() */
+}
diff --git a/apply.h b/apply.h
index 9a98eae..77502be 100644
--- a/apply.h
+++ b/apply.h
@@ -97,4 +97,14 @@ struct apply_state {
int applied_after_fixing_ws;
 };
 
+extern int parse_whitespace_option(struct apply_state *state,
+  const char *option);
+extern int parse_ignorewhitespace_option(struct apply_state *state,
+const char *option);
+
+extern void init_apply_state(struct apply_state *state,
+const char *prefix,
+struct lock_file *lock_file);
+extern void clear_apply_state(struct apply_state *state);
+
 #endif
diff --git a/builtin/apply.c b/builtin/apply.c
index e2f970d..bc15545 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -27,52 +27,6 @@ static const char * const 

[PATCH v6 13/44] builtin/apply: make apply_all_patches() return -1 on error

[Christian Couder]

To finish libifying the apply functionality, apply_all_patches() should not
die() or exit() in case of error, but return -1.

While doing that we must take care that file descriptors are properly closed
and, if needed, reset a sensible value.

Also, according to the lockfile API, when finished with a lockfile, one
should either commit it or roll it back.

This is even more important now that the same lockfile can be passed
to init_apply_state() many times to be reused by series of calls to
the apply lib functions.

Helped-by: Eric Sunshine 
Helped-by: Nguyễn Thái Ngọc Duy 
Helped-by: Johannes Schindelin 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 40 +++-
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index a27fdd3..c27be35 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4558,7 +4558,7 @@ static int apply_all_patches(struct apply_state *state,
if (!strcmp(arg, "-")) {
res = apply_patch(state, 0, "", options);
if (res < 0)
-   exit(1);
+   goto rollback_end;
errs |= res;
read_stdin = 0;
continue;
@@ -4568,21 +4568,23 @@ static int apply_all_patches(struct apply_state *state,
  arg);
 
fd = open(arg, O_RDONLY);
-   if (fd < 0)
-   die_errno(_("can't open patch '%s'"), arg);
+   if (fd < 0) {
+   error(_("can't open patch '%s': %s"), arg, 
strerror(errno));
+   goto rollback_end;
+   }
read_stdin = 0;
set_default_whitespace_mode(state);
res = apply_patch(state, fd, arg, options);
+   close(fd);
if (res < 0)
-   exit(1);
+   goto rollback_end;
errs |= res;
-   close(fd);
}
set_default_whitespace_mode(state);
if (read_stdin) {
res = apply_patch(state, 0, "", options);
if (res < 0)
-   exit(1);
+   goto rollback_end;
errs |= res;
}
 
@@ -4596,11 +4598,13 @@ static int apply_all_patches(struct apply_state *state,
   squelched),
squelched);
}
-   if (state->ws_error_action == die_on_ws_error)
-   die(Q_("%d line adds whitespace errors.",
-  "%d lines add whitespace errors.",
-  state->whitespace_error),
-   state->whitespace_error);
+   if (state->ws_error_action == die_on_ws_error) {
+   error(Q_("%d line adds whitespace errors.",
+"%d lines add whitespace errors.",
+state->whitespace_error),
+ state->whitespace_error);
+   goto rollback_end;
+   }
if (state->applied_after_fixing_ws && state->apply)
warning("%d line%s applied after"
" fixing whitespace errors.",
@@ -4614,12 +4618,22 @@ static int apply_all_patches(struct apply_state *state,
}
 
if (state->update_index) {
-   if (write_locked_index(_index, state->lock_file, 
COMMIT_LOCK))
-   die(_("Unable to write new index file"));
+   res = write_locked_index(_index, state->lock_file, 
COMMIT_LOCK);
+   if (res) {
+   error(_("Unable to write new index file"));
+   goto rollback_end;
+   }
state->newfd = -1;
}
 
return !!errs;
+
+rollback_end:
+   if (state->newfd >= 0) {
+   rollback_lock_file(state->lock_file);
+   state->newfd = -1;
+   }
+   return -1;
 }
 
 int cmd_apply(int argc, const char **argv, const char *prefix)
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 05/44] builtin/apply: make parse_chunk() return a negative integer on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing or exit()ing.

To do that in a compatible manner with the rest of the error handling
in builtin/apply.c, parse_chunk() should return -1 instead of calling
die() or exit().

As parse_chunk() is called only by apply_patch() which already
returns -1 when an error happened, let's make apply_patch() return -1
when parse_chunk() returns -1.

If find_header() returns -2 because no patch header has been found, it
is ok for parse_chunk() to also return -2. If find_header() returns -1
because an error happened, it is ok for parse_chunk() to do the same.

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 630c01c..a160338 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1991,22 +1991,22 @@ static int use_patch(struct apply_state *state, struct 
patch *p)
return !state->has_include;
 }
 
-
 /*
  * Read the patch text in "buffer" that extends for "size" bytes; stop
  * reading after seeing a single patch (i.e. changes to a single file).
  * Create fragments (i.e. patch hunks) and hang them to the given patch.
- * Return the number of bytes consumed, so that the caller can call us
- * again for the next patch.
+ *
+ * Returns:
+ *   -1 on error,
+ *   -2 if no header was found,
+ *   the number of bytes consumed otherwise,
+ * so that the caller can call us again for the next patch.
  */
 static int parse_chunk(struct apply_state *state, char *buffer, unsigned long 
size, struct patch *patch)
 {
int hdrsize, patchsize;
int offset = find_header(state, buffer, size, , patch);
 
-   if (offset == -1)
-   exit(1);
-
if (offset < 0)
return offset;
 
@@ -2067,7 +2067,7 @@ static int parse_chunk(struct apply_state *state, char 
*buffer, unsigned long si
 */
if ((state->apply || state->check) &&
(!patch->is_binary && !metadata_changes(patch)))
-   die(_("patch with only garbage at line %d"), 
state->linenr);
+   return error(_("patch with only garbage at line %d"), 
state->linenr);
}
 
return offset + hdrsize + patchsize;
@@ -4449,6 +4449,10 @@ static int apply_patch(struct apply_state *state,
nr = parse_chunk(state, buf.buf + offset, buf.len - offset, 
patch);
if (nr < 0) {
free_patch(patch);
+   if (nr == -1) {
+   res = -1;
+   goto end;
+   }
break;
}
if (state->apply_in_reverse)
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 18/44] builtin/apply: make build_fake_ancestor() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", build_fake_ancestor() should return -1 instead
of calling die().

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 41 ++---
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 429fddd..e74b068 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3889,11 +3889,12 @@ static int preimage_sha1_in_gitlink_patch(struct patch 
*p, unsigned char sha1[20
 }
 
 /* Build an index that contains the just the files needed for a 3way merge */
-static void build_fake_ancestor(struct patch *list, const char *filename)
+static int build_fake_ancestor(struct patch *list, const char *filename)
 {
struct patch *patch;
struct index_state result = { NULL };
static struct lock_file lock;
+   int res;
 
/* Once we start supporting the reverse patch, it may be
 * worth showing the new sha1 prefix, but until then...
@@ -3911,31 +3912,38 @@ static void build_fake_ancestor(struct patch *list, 
const char *filename)
if (!preimage_sha1_in_gitlink_patch(patch, sha1))
; /* ok, the textual part looks sane */
else
-   die("sha1 information is lacking or useless for 
submodule %s",
-   name);
+   return error("sha1 information is lacking or "
+"useless for submodule %s", name);
} else if (!get_sha1_blob(patch->old_sha1_prefix, sha1)) {
; /* ok */
} else if (!patch->lines_added && !patch->lines_deleted) {
/* mode-only change: update the current */
if (get_current_sha1(patch->old_name, sha1))
-   die("mode change for %s, which is not "
-   "in current HEAD", name);
+   return error("mode change for %s, which is not "
+"in current HEAD", name);
} else
-   die("sha1 information is lacking or useless "
-   "(%s).", name);
+   return error("sha1 information is lacking or useless "
+"(%s).", name);
 
ce = make_cache_entry(patch->old_mode, sha1, name, 0, 0);
if (!ce)
-   die(_("make_cache_entry failed for path '%s'"), name);
-   if (add_index_entry(, ce, ADD_CACHE_OK_TO_ADD))
-   die ("Could not add %s to temporary index", name);
+   return error(_("make_cache_entry failed for path '%s'"),
+name);
+   if (add_index_entry(, ce, ADD_CACHE_OK_TO_ADD)) {
+   free(ce);
+   return error("Could not add %s to temporary index",
+name);
+   }
}
 
hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
-   if (write_locked_index(, , COMMIT_LOCK))
-   die ("Could not write temporary index to %s", filename);
-
+   res = write_locked_index(, , COMMIT_LOCK);
discard_index();
+
+   if (res)
+   return error("Could not write temporary index to %s", filename);
+
+   return 0;
 }
 
 static void stat_patch_list(struct apply_state *state, struct patch *patch)
@@ -4476,8 +4484,11 @@ static int apply_patch(struct apply_state *state,
goto end;
}
 
-   if (state->fake_ancestor)
-   build_fake_ancestor(list, state->fake_ancestor);
+   if (state->fake_ancestor &&
+   build_fake_ancestor(list, state->fake_ancestor)) {
+   res = -1;
+   goto end;
+   }
 
if (state->diffstat)
stat_patch_list(state, list);
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 03/44] builtin/apply: read_patch_file() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing. Let's do that by returning -1 instead of
die()ing in read_patch_file().

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 598e479..2ff8450 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -335,10 +335,10 @@ static void say_patch_name(FILE *output, const char *fmt, 
struct patch *patch)
 
 #define SLOP (16)
 
-static void read_patch_file(struct strbuf *sb, int fd)
+static int read_patch_file(struct strbuf *sb, int fd)
 {
if (strbuf_read(sb, fd, 0) < 0)
-   die_errno("git apply: failed to read");
+   return error_errno("git apply: failed to read");
 
/*
 * Make sure that we have some slop in the buffer
@@ -347,6 +347,7 @@ static void read_patch_file(struct strbuf *sb, int fd)
 */
strbuf_grow(sb, SLOP);
memset(sb->buf + sb->len, 0, SLOP);
+   return 0;
 }
 
 static unsigned long linelen(const char *buffer, unsigned long size)
@@ -4424,7 +4425,8 @@ static int apply_patch(struct apply_state *state,
int res = 0;
 
state->patch_input_file = filename;
-   read_patch_file(, fd);
+   if (read_patch_file(, fd))
+   return -1;
offset = 0;
while (offset < buf.len) {
struct patch *patch;
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 11/44] builtin/apply: make check_apply_state() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", check_apply_state() should return -1 instead of
calling die().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 2ae1243..d60ffce 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4541,17 +4541,17 @@ static int option_parse_directory(const struct option 
*opt,
return 0;
 }
 
-static void check_apply_state(struct apply_state *state, int force_apply)
+static int check_apply_state(struct apply_state *state, int force_apply)
 {
int is_not_gitdir = !startup_info->have_repository;
 
if (state->apply_with_reject && state->threeway)
-   die("--reject and --3way cannot be used together.");
+   return error("--reject and --3way cannot be used together.");
if (state->cached && state->threeway)
-   die("--cached and --3way cannot be used together.");
+   return error("--cached and --3way cannot be used together.");
if (state->threeway) {
if (is_not_gitdir)
-   die(_("--3way outside a repository"));
+   return error(_("--3way outside a repository"));
state->check_index = 1;
}
if (state->apply_with_reject)
@@ -4559,16 +4559,18 @@ static void check_apply_state(struct apply_state 
*state, int force_apply)
if (!force_apply && (state->diffstat || state->numstat || 
state->summary || state->check || state->fake_ancestor))
state->apply = 0;
if (state->check_index && is_not_gitdir)
-   die(_("--index outside a repository"));
+   return error(_("--index outside a repository"));
if (state->cached) {
if (is_not_gitdir)
-   die(_("--cached outside a repository"));
+   return error(_("--cached outside a repository"));
state->check_index = 1;
}
if (state->check_index)
state->unsafe_paths = 0;
if (!state->lock_file)
-   die("BUG: state->lock_file should not be NULL");
+   return error("BUG: state->lock_file should not be NULL");
+
+   return 0;
 }
 
 static int apply_all_patches(struct apply_state *state,
@@ -4734,7 +4736,8 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, state.prefix, builtin_apply_options,
apply_usage, 0);
 
-   check_apply_state(, force_apply);
+   if (check_apply_state(, force_apply))
+   exit(1);
 
ret = apply_all_patches(, argc, argv, options);
 
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 34/44] write_or_die: use warning() instead of fprintf(stderr, ...)

[Christian Couder]

Signed-off-by: Christian Couder 
---
 write_or_die.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/write_or_die.c b/write_or_die.c
index 49e80aa..c29f677 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -87,8 +87,7 @@ int write_or_whine_pipe(int fd, const void *buf, size_t 
count, const char *msg)
 {
if (write_in_full(fd, buf, count) < 0) {
check_pipe(errno);
-   fprintf(stderr, "%s: write error (%s)\n",
-   msg, strerror(errno));
+   warning("%s: write error (%s)\n", msg, strerror(errno));
return 0;
}
 
@@ -98,8 +97,7 @@ int write_or_whine_pipe(int fd, const void *buf, size_t 
count, const char *msg)
 int write_or_whine(int fd, const void *buf, size_t count, const char *msg)
 {
if (write_in_full(fd, buf, count) < 0) {
-   fprintf(stderr, "%s: write error (%s)\n",
-   msg, strerror(errno));
+   warning("%s: write error (%s)\n", msg, strerror(errno));
return 0;
}
 
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 38/44] usage: add set_warn_routine()

[Christian Couder]

There are already set_die_routine() and set_error_routine(),
so let's add set_warn_routine() as this will be needed in a
following commit.

Signed-off-by: Christian Couder 
---
 git-compat-util.h | 1 +
 usage.c   | 5 +
 2 files changed, 6 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 49d4029..f78706a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -440,6 +440,7 @@ static inline int const_error(void)
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, 
va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list 
params));
+extern void set_warn_routine(void (*routine)(const char *warn, va_list 
params));
 extern void set_die_is_recursing_routine(int (*routine)(void));
 extern void set_error_handle(FILE *);
 
diff --git a/usage.c b/usage.c
index 1dad03f..67e5526 100644
--- a/usage.c
+++ b/usage.c
@@ -70,6 +70,11 @@ void set_error_routine(void (*routine)(const char *err, 
va_list params))
error_routine = routine;
 }
 
+void set_warn_routine(void (*routine)(const char *warn, va_list params))
+{
+   warn_routine = routine;
+}
+
 void set_die_is_recursing_routine(int (*routine)(void))
 {
die_is_recursing = routine;
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 42/44] run-command: make dup_devnull() static again

[Christian Couder]

As there is no caller of dup_devnull() outside run-command.c any more,
let's make dup_devnull() static again.

Signed-off-by: Christian Couder 
---
 run-command.c | 2 +-
 run-command.h | 6 --
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/run-command.c b/run-command.c
index d8ed88c..c09d6b0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
 }
 
 #ifndef GIT_WINDOWS_NATIVE
-void dup_devnull(int to)
+static void dup_devnull(int to)
 {
int fd = open("/dev/null", O_RDWR);
if (fd < 0)
diff --git a/run-command.h b/run-command.h
index e05ce7d..11f76b0 100644
--- a/run-command.h
+++ b/run-command.h
@@ -201,10 +201,4 @@ int run_processes_parallel(int n,
   task_finished_fn,
   void *pp_cb);
 
-/**
- * Misc helper functions
- */
-
-void dup_devnull(int to);
-
 #endif
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 39/44] usage: add get_error_routine() and get_warn_routine()

[Christian Couder]

Let's make it possible to get the current error_routine and warn_routine,
so that we can store them before using set_error_routine() or
set_warn_routine() to use new ones.

This way we will be able put back the original routines, when we are done
with using new ones.

Signed-off-by: Christian Couder 
---
 git-compat-util.h |  2 ++
 usage.c   | 10 ++
 2 files changed, 12 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index f78706a..92af27d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -440,7 +440,9 @@ static inline int const_error(void)
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, 
va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list 
params));
+extern void (*get_error_routine(void))(const char *err, va_list params);
 extern void set_warn_routine(void (*routine)(const char *warn, va_list 
params));
+extern void (*get_warn_routine(void))(const char *warn, va_list params);
 extern void set_die_is_recursing_routine(int (*routine)(void));
 extern void set_error_handle(FILE *);
 
diff --git a/usage.c b/usage.c
index 67e5526..2fd3045 100644
--- a/usage.c
+++ b/usage.c
@@ -70,11 +70,21 @@ void set_error_routine(void (*routine)(const char *err, 
va_list params))
error_routine = routine;
 }
 
+void (*get_error_routine(void))(const char *err, va_list params)
+{
+   return error_routine;
+}
+
 void set_warn_routine(void (*routine)(const char *warn, va_list params))
 {
warn_routine = routine;
 }
 
+void (*get_warn_routine(void))(const char *warn, va_list params)
+{
+   return warn_routine;
+}
+
 void set_die_is_recursing_routine(int (*routine)(void))
 {
die_is_recursing = routine;
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 31/44] run-command: make dup_devnull() non static

[Christian Couder]

We will need this function in a later commit to redirect stdout
and stderr to /dev/null.

Helped-by: Johannes Sixt 
Helped-by: Johannes Schindelin 
Signed-off-by: Christian Couder 
---
 run-command.c | 2 +-
 run-command.h | 6 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index af0c8a1..d8ed88c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -85,7 +85,7 @@ static inline void close_pair(int fd[2])
 }
 
 #ifndef GIT_WINDOWS_NATIVE
-static inline void dup_devnull(int to)
+void dup_devnull(int to)
 {
int fd = open("/dev/null", O_RDWR);
if (fd < 0)
diff --git a/run-command.h b/run-command.h
index 11f76b0..e05ce7d 100644
--- a/run-command.h
+++ b/run-command.h
@@ -201,4 +201,10 @@ int run_processes_parallel(int n,
   task_finished_fn,
   void *pp_cb);
 
+/**
+ * Misc helper functions
+ */
+
+void dup_devnull(int to);
+
 #endif
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 14/44] builtin/apply: make parse_traditional_patch() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", parse_traditional_patch() should return -1
instead of calling die().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index c27be35..eb98116 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -755,10 +755,10 @@ static int has_epoch_timestamp(const char *nameline)
  * files, we can happily check the index for a match, but for creating a
  * new file we should try to match whatever "patch" does. I have no idea.
  */
-static void parse_traditional_patch(struct apply_state *state,
-   const char *first,
-   const char *second,
-   struct patch *patch)
+static int parse_traditional_patch(struct apply_state *state,
+  const char *first,
+  const char *second,
+  struct patch *patch)
 {
char *name;
 
@@ -803,7 +803,9 @@ static void parse_traditional_patch(struct apply_state 
*state,
}
}
if (!name)
-   die(_("unable to find filename in patch at line %d"), 
state->linenr);
+   return error(_("unable to find filename in patch at line %d"), 
state->linenr);
+
+   return 0;
 }
 
 static int gitdiff_hdrend(struct apply_state *state,
@@ -1462,7 +1464,8 @@ static int find_header(struct apply_state *state,
continue;
 
/* Ok, we'll consider it a patch */
-   parse_traditional_patch(state, line, line+len, patch);
+   if (parse_traditional_patch(state, line, line+len, patch))
+   return -1;
*hdrsize = len + nextlen;
state->linenr += 2;
return offset;
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 12/44] builtin/apply: move check_apply_state() to apply.c

[Christian Couder]

To libify `git apply` functionality we must make check_apply_state()
usable outside "builtin/apply.c".

Let's do that by moving it into "apply.c".

Signed-off-by: Christian Couder 
---
 apply.c | 32 
 apply.h |  1 +
 builtin/apply.c | 32 
 3 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/apply.c b/apply.c
index c5a9ee2..84dae3d 100644
--- a/apply.c
+++ b/apply.c
@@ -90,3 +90,35 @@ void clear_apply_state(struct apply_state *state)
 
/* >fn_table is cleared at the end of apply_patch() */
 }
+
+int check_apply_state(struct apply_state *state, int force_apply)
+{
+   int is_not_gitdir = !startup_info->have_repository;
+
+   if (state->apply_with_reject && state->threeway)
+   return error("--reject and --3way cannot be used together.");
+   if (state->cached && state->threeway)
+   return error("--cached and --3way cannot be used together.");
+   if (state->threeway) {
+   if (is_not_gitdir)
+   return error(_("--3way outside a repository"));
+   state->check_index = 1;
+   }
+   if (state->apply_with_reject)
+   state->apply = state->apply_verbosely = 1;
+   if (!force_apply && (state->diffstat || state->numstat || 
state->summary || state->check || state->fake_ancestor))
+   state->apply = 0;
+   if (state->check_index && is_not_gitdir)
+   return error(_("--index outside a repository"));
+   if (state->cached) {
+   if (is_not_gitdir)
+   return error(_("--cached outside a repository"));
+   state->check_index = 1;
+   }
+   if (state->check_index)
+   state->unsafe_paths = 0;
+   if (!state->lock_file)
+   return error("BUG: state->lock_file should not be NULL");
+
+   return 0;
+}
diff --git a/apply.h b/apply.h
index 7d3a03b..1f2277e 100644
--- a/apply.h
+++ b/apply.h
@@ -106,5 +106,6 @@ extern int init_apply_state(struct apply_state *state,
const char *prefix,
struct lock_file *lock_file);
 extern void clear_apply_state(struct apply_state *state);
+extern int check_apply_state(struct apply_state *state, int force_apply);
 
 #endif
diff --git a/builtin/apply.c b/builtin/apply.c
index d60ffce..a27fdd3 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4541,38 +4541,6 @@ static int option_parse_directory(const struct option 
*opt,
return 0;
 }
 
-static int check_apply_state(struct apply_state *state, int force_apply)
-{
-   int is_not_gitdir = !startup_info->have_repository;
-
-   if (state->apply_with_reject && state->threeway)
-   return error("--reject and --3way cannot be used together.");
-   if (state->cached && state->threeway)
-   return error("--cached and --3way cannot be used together.");
-   if (state->threeway) {
-   if (is_not_gitdir)
-   return error(_("--3way outside a repository"));
-   state->check_index = 1;
-   }
-   if (state->apply_with_reject)
-   state->apply = state->apply_verbosely = 1;
-   if (!force_apply && (state->diffstat || state->numstat || 
state->summary || state->check || state->fake_ancestor))
-   state->apply = 0;
-   if (state->check_index && is_not_gitdir)
-   return error(_("--index outside a repository"));
-   if (state->cached) {
-   if (is_not_gitdir)
-   return error(_("--cached outside a repository"));
-   state->check_index = 1;
-   }
-   if (state->check_index)
-   state->unsafe_paths = 0;
-   if (!state->lock_file)
-   return error("BUG: state->lock_file should not be NULL");
-
-   return 0;
-}
-
 static int apply_all_patches(struct apply_state *state,
 int argc,
 const char **argv,
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 35/44] apply: add 'be_silent' variable to 'struct apply_state'

[Christian Couder]

This variable should prevent anything to be printed on both stderr
and stdout.

Let's not take care of stdout and apply_verbosely for now though,
as that will be taken care of in following patches.

Signed-off-by: Christian Couder 
---
 apply.c | 43 +--
 apply.h |  1 +
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/apply.c b/apply.c
index 713d1c0..dbb2515 100644
--- a/apply.c
+++ b/apply.c
@@ -1612,8 +1612,9 @@ static void record_ws_error(struct apply_state *state,
return;
 
err = whitespace_error_string(result);
-   fprintf(stderr, "%s:%d: %s.\n%.*s\n",
-   state->patch_input_file, linenr, err, len, line);
+   if (!state->be_silent)
+   fprintf(stderr, "%s:%d: %s.\n%.*s\n",
+   state->patch_input_file, linenr, err, len, line);
free(err);
 }
 
@@ -1808,7 +1809,7 @@ static int parse_single_patch(struct apply_state *state,
return error(_("new file %s depends on old contents"), 
patch->new_name);
if (0 < patch->is_delete && newlines)
return error(_("deleted file %s still has contents"), 
patch->old_name);
-   if (!patch->is_delete && !newlines && context)
+   if (!patch->is_delete && !newlines && context && !state->be_silent)
fprintf_ln(stderr,
   _("** warning: "
 "file %s becomes empty but is not deleted"),
@@ -3031,8 +3032,8 @@ static int apply_one_fragment(struct apply_state *state,
 * Warn if it was necessary to reduce the number
 * of context lines.
 */
-   if ((leading != frag->leading) ||
-   (trailing != frag->trailing))
+   if ((leading != frag->leading ||
+trailing != frag->trailing) && !state->be_silent)
fprintf_ln(stderr, _("Context reduced to (%ld/%ld)"
 " to apply fragment at %d"),
   leading, trailing, applied_pos+1);
@@ -3529,7 +3530,8 @@ static int try_threeway(struct apply_state *state,
 read_blob_object(, pre_sha1, patch->old_mode))
return error("repository lacks the necessary blob to fall back 
on 3-way merge.");
 
-   fprintf(stderr, "Falling back to three-way merge...\n");
+   if (!state->be_silent)
+   fprintf(stderr, "Falling back to three-way merge...\n");
 
img = strbuf_detach(, );
prepare_image(_image, img, len, 1);
@@ -3559,7 +3561,9 @@ static int try_threeway(struct apply_state *state,
status = three_way_merge(image, patch->new_name,
 pre_sha1, our_sha1, post_sha1);
if (status < 0) {
-   fprintf(stderr, "Failed to fall back on three-way merge...\n");
+   if (!state->be_silent)
+   fprintf(stderr,
+   "Failed to fall back on three-way merge...\n");
return status;
}
 
@@ -3571,9 +3575,15 @@ static int try_threeway(struct apply_state *state,
hashcpy(patch->threeway_stage[0].hash, pre_sha1);
hashcpy(patch->threeway_stage[1].hash, our_sha1);
hashcpy(patch->threeway_stage[2].hash, post_sha1);
-   fprintf(stderr, "Applied patch to '%s' with conflicts.\n", 
patch->new_name);
+   if (!state->be_silent)
+   fprintf(stderr,
+   "Applied patch to '%s' with conflicts.\n",
+   patch->new_name);
} else {
-   fprintf(stderr, "Applied patch to '%s' cleanly.\n", 
patch->new_name);
+   if (!state->be_silent)
+   fprintf(stderr,
+   "Applied patch to '%s' cleanly.\n",
+   patch->new_name);
}
return 0;
 }
@@ -4472,7 +4482,8 @@ static int write_out_one_reject(struct apply_state 
*state, struct patch *patch)
"Applying patch %%s with %d rejects...",
cnt),
cnt);
-   say_patch_name(stderr, sb.buf, patch);
+   if (!state->be_silent)
+   say_patch_name(stderr, sb.buf, patch);
strbuf_release();
 
cnt = strlen(patch->new_name);
@@ -4499,10 +4510,12 @@ static int write_out_one_reject(struct apply_state 
*state, struct patch *patch)
 frag;
 cnt++, frag = frag->next) {
if (!frag->rejected) {
-   fprintf_ln(stderr, _("Hunk #%d applied cleanly."), cnt);
+   if (!state->be_silent)
+   fprintf_ln(stderr, _("Hunk #%d applied 
cleanly."), cnt);
continue;
}
-   fprintf_ln(stderr, 

[PATCH v6 40/44] apply: change error_routine when be_silent is set

[Christian Couder]

Signed-off-by: Christian Couder 
---
 apply.c | 23 +--
 apply.h |  4 
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index 2529534..ef49709 100644
--- a/apply.c
+++ b/apply.c
@@ -109,6 +109,11 @@ void clear_apply_state(struct apply_state *state)
/* >fn_table is cleared at the end of apply_patch() */
 }
 
+static void mute_routine(const char *bla, va_list params)
+{
+   /* do nothing */
+}
+
 int check_apply_state(struct apply_state *state, int force_apply)
 {
int is_not_gitdir = !startup_info->have_repository;
@@ -143,6 +148,13 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
if (!state->lock_file)
return error("BUG: state->lock_file should not be NULL");
 
+   if (state->be_silent) {
+   state->saved_error_routine = get_error_routine();
+   state->saved_warn_routine = get_warn_routine();
+   set_error_routine(mute_routine);
+   set_warn_routine(mute_routine);
+   }
+
return 0;
 }
 
@@ -4760,6 +4772,7 @@ int apply_all_patches(struct apply_state *state,
 {
int i;
int res;
+   int retval = -1;
int errs = 0;
int read_stdin = 1;
 
@@ -4838,12 +4851,18 @@ int apply_all_patches(struct apply_state *state,
state->newfd = -1;
}
 
-   return !!errs;
+   retval = !!errs;
 
 rollback_end:
if (state->newfd >= 0) {
rollback_lock_file(state->lock_file);
state->newfd = -1;
}
-   return -1;
+
+   if (state->be_silent) {
+   set_error_routine(state->saved_error_routine);
+   set_warn_routine(state->saved_warn_routine);
+   }
+
+   return retval;
 }
diff --git a/apply.h b/apply.h
index 034541a..c6cf33d 100644
--- a/apply.h
+++ b/apply.h
@@ -89,6 +89,10 @@ struct apply_state {
 */
struct string_list fn_table;
 
+   /* This is to save some reporting routines */
+   void (*saved_error_routine)(const char *err, va_list params);
+   void (*saved_warn_routine)(const char *warn, va_list params);
+
/* These control whitespace errors */
enum ws_error_action ws_error_action;
enum ws_ignore ws_ignore_action;
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 41/44] am: use be_silent in 'struct apply_state' to shut up applying patches

[Christian Couder]

Signed-off-by: Christian Couder 
---
 builtin/am.c | 29 -
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index a16b06c..43f7316 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1525,7 +1525,6 @@ static int run_apply(const struct am_state *state, const 
char *index_file)
struct argv_array apply_paths = ARGV_ARRAY_INIT;
struct argv_array apply_opts = ARGV_ARRAY_INIT;
struct apply_state apply_state;
-   int save_stdout_fd, save_stderr_fd;
int res, opts_left;
char *save_index_file;
static struct lock_file lock_file;
@@ -1559,18 +1558,6 @@ static int run_apply(const struct am_state *state, const 
char *index_file)
OPT_END()
};
 
-   /*
-* If we are allowed to fall back on 3-way merge, don't give false
-* errors during the initial attempt.
-*/
-
-   if (state->threeway && !index_file) {
-   save_stdout_fd = dup(1);
-   dup_devnull(1);
-   save_stderr_fd = dup(2);
-   dup_devnull(2);
-   }
-
if (index_file) {
save_index_file = get_index_file();
set_index_file((char *)index_file);
@@ -1593,6 +1580,14 @@ static int run_apply(const struct am_state *state, const 
char *index_file)
else
apply_state.check_index = 1;
 
+   /*
+* If we are allowed to fall back on 3-way merge, don't give false
+* errors during the initial attempt.
+*/
+
+   if (state->threeway && !index_file)
+   apply_state.be_silent = 1;
+
if (check_apply_state(_state, 0))
die("check_apply_state() failed");
 
@@ -1600,14 +1595,6 @@ static int run_apply(const struct am_state *state, const 
char *index_file)
 
res = apply_all_patches(_state, apply_paths.argc, 
apply_paths.argv, 0);
 
-   /* Restore stdout and stderr */
-   if (state->threeway && !index_file) {
-   dup2(save_stdout_fd, 1);
-   close(save_stdout_fd);
-   dup2(save_stderr_fd, 2);
-   close(save_stderr_fd);
-   }
-
if (index_file)
set_index_file(save_index_file);
 
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 30/44] apply: make some parsing functions static again

[Christian Couder]

Some parsing functions that were used in both "apply.c" and
"builtin/apply.c" are now only used in the former, so they
can be made static to "apply.c".

Signed-off-by: Christian Couder 
---
 apply.c | 6 +++---
 apply.h | 5 -
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/apply.c b/apply.c
index 4920fa8..713d1c0 100644
--- a/apply.c
+++ b/apply.c
@@ -27,7 +27,7 @@ static void git_apply_config(void)
git_config(git_default_config, NULL);
 }
 
-int parse_whitespace_option(struct apply_state *state, const char *option)
+static int parse_whitespace_option(struct apply_state *state, const char 
*option)
 {
if (!option) {
state->ws_error_action = warn_on_ws_error;
@@ -57,8 +57,8 @@ int parse_whitespace_option(struct apply_state *state, const 
char *option)
return error(_("unrecognized whitespace option '%s'"), option);
 }
 
-int parse_ignorewhitespace_option(struct apply_state *state,
- const char *option)
+static int parse_ignorewhitespace_option(struct apply_state *state,
+const char *option)
 {
if (!option || !strcmp(option, "no") ||
!strcmp(option, "false") || !strcmp(option, "never") ||
diff --git a/apply.h b/apply.h
index 736f818..89e7982 100644
--- a/apply.h
+++ b/apply.h
@@ -97,11 +97,6 @@ struct apply_state {
int applied_after_fixing_ws;
 };
 
-extern int parse_whitespace_option(struct apply_state *state,
-  const char *option);
-extern int parse_ignorewhitespace_option(struct apply_state *state,
-const char *option);
-
 extern int apply_option_parse_exclude(const struct option *opt,
  const char *arg, int unset);
 extern int apply_option_parse_include(const struct option *opt,
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 24/44] builtin/apply: make write_out_results() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of exit()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", write_out_results() should return -1 instead of
calling exit().

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 291e24e..f35c901 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4372,6 +4372,12 @@ static int write_out_one_reject(struct apply_state 
*state, struct patch *patch)
return -1;
 }
 
+/*
+ * Returns:
+ *  -1 if an error happened
+ *   0 if the patch applied cleanly
+ *   1 if the patch did not apply cleanly
+ */
 static int write_out_results(struct apply_state *state, struct patch *list)
 {
int phase;
@@ -4385,8 +4391,10 @@ static int write_out_results(struct apply_state *state, 
struct patch *list)
if (l->rejected)
errs = 1;
else {
-   if (write_out_one_result(state, l, phase))
-   exit(1);
+   if (write_out_one_result(state, l, phase)) {
+   string_list_clear(, 0);
+   return -1;
+   }
if (phase == 1) {
if (write_out_one_reject(state, l))
errs = 1;
@@ -4498,10 +4506,17 @@ static int apply_patch(struct apply_state *state,
goto end;
}
 
-   if (state->apply && write_out_results(state, list)) {
-   /* with --3way, we still need to write the index out */
-   res = state->apply_with_reject ? -1 : 1;
-   goto end;
+   if (state->apply) {
+   int write_res = write_out_results(state, list);
+   if (write_res < 0) {
+   res = -1;
+   goto end;
+   }
+   if (write_res > 0) {
+   /* with --3way, we still need to write the index out */
+   res = state->apply_with_reject ? -1 : 1;
+   goto end;
+   }
}
 
if (state->fake_ancestor &&
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 06/44] builtin/apply: make parse_single_patch() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in builtin/apply.c, parse_single_patch() should return -1 instead of
calling die().

Let's do that by using error() and let's adjust the related test
cases accordingly.

Signed-off-by: Christian Couder 
---
 builtin/apply.c| 17 +
 t/t4012-diff-binary.sh |  4 ++--
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index a160338..2671586 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1666,6 +1666,10 @@ static int parse_fragment(struct apply_state *state,
  *
  * The (fragment->patch, fragment->size) pair points into the memory given
  * by the caller, not a copy, when we return.
+ *
+ * Returns:
+ *   -1 in case of error,
+ *   the number of bytes in the patch otherwise.
  */
 static int parse_single_patch(struct apply_state *state,
  const char *line,
@@ -1683,8 +1687,10 @@ static int parse_single_patch(struct apply_state *state,
fragment = xcalloc(1, sizeof(*fragment));
fragment->linenr = state->linenr;
len = parse_fragment(state, line, size, patch, fragment);
-   if (len <= 0)
-   die(_("corrupt patch at line %d"), state->linenr);
+   if (len <= 0) {
+   free(fragment);
+   return error(_("corrupt patch at line %d"), 
state->linenr);
+   }
fragment->patch = line;
fragment->size = len;
oldlines += fragment->oldlines;
@@ -1720,9 +1726,9 @@ static int parse_single_patch(struct apply_state *state,
patch->is_delete = 0;
 
if (0 < patch->is_new && oldlines)
-   die(_("new file %s depends on old contents"), patch->new_name);
+   return error(_("new file %s depends on old contents"), 
patch->new_name);
if (0 < patch->is_delete && newlines)
-   die(_("deleted file %s still has contents"), patch->old_name);
+   return error(_("deleted file %s still has contents"), 
patch->old_name);
if (!patch->is_delete && !newlines && context)
fprintf_ln(stderr,
   _("** warning: "
@@ -2024,6 +2030,9 @@ static int parse_chunk(struct apply_state *state, char 
*buffer, unsigned long si
   size - offset - hdrsize,
   patch);
 
+   if (patchsize < 0)
+   return -1;
+
if (!patchsize) {
static const char git_binary[] = "GIT binary patch\n";
int hd = hdrsize + offset;
diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
index 643d729..0a8af76 100755
--- a/t/t4012-diff-binary.sh
+++ b/t/t4012-diff-binary.sh
@@ -68,7 +68,7 @@ test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt 
patch correctly' '
sed -e "s/-CIT/xCIT/" broken &&
test_must_fail git apply --stat --summary broken 2>detected &&
detected=$(cat detected) &&
-   detected=$(expr "$detected" : "fatal.*at line \\([0-9]*\\)\$") &&
+   detected=$(expr "$detected" : "error.*at line \\([0-9]*\\)\$") &&
detected=$(sed -ne "${detected}p" broken) &&
test "$detected" = xCIT
 '
@@ -77,7 +77,7 @@ test_expect_success C_LOCALE_OUTPUT 'apply detecting corrupt 
patch correctly' '
git diff --binary | sed -e "s/-CIT/xCIT/" >broken &&
test_must_fail git apply --stat --summary broken 2>detected &&
detected=$(cat detected) &&
-   detected=$(expr "$detected" : "fatal.*at line \\([0-9]*\\)\$") &&
+   detected=$(expr "$detected" : "error.*at line \\([0-9]*\\)\$") &&
detected=$(sed -ne "${detected}p" broken) &&
test "$detected" = xCIT
 '
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 25/44] builtin/apply: make try_create_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", try_create_file() should return -1 in case of
error.

Unfortunately try_create_file() currently returns -1 to signal a
recoverable error. To fix that, let's make it return 1 in case of
a recoverable error and -1 in case of an unrecoverable error.

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 41 ++---
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index f35c901..37f0c2e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4139,38 +4139,45 @@ static int add_index_file(struct apply_state *state,
return 0;
 }
 
+/*
+ * Returns:
+ *  -1 if an unrecoverable error happened
+ *   0 if everything went well
+ *   1 if a recoverable error happened
+ */
 static int try_create_file(const char *path, unsigned int mode, const char 
*buf, unsigned long size)
 {
-   int fd;
+   int fd, res;
struct strbuf nbuf = STRBUF_INIT;
 
if (S_ISGITLINK(mode)) {
struct stat st;
if (!lstat(path, ) && S_ISDIR(st.st_mode))
return 0;
-   return mkdir(path, 0777);
+   return !!mkdir(path, 0777);
}
 
if (has_symlinks && S_ISLNK(mode))
/* Although buf:size is counted string, it also is NUL
 * terminated.
 */
-   return symlink(buf, path);
+   return !!symlink(buf, path);
 
fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode & 0100) ? 0777 : 
0666);
if (fd < 0)
-   return -1;
+   return 1;
 
if (convert_to_working_tree(path, buf, size, )) {
size = nbuf.len;
buf  = nbuf.buf;
}
-   write_or_die(fd, buf, size);
+   res = !write_or_whine_pipe(fd, buf, size, path);
strbuf_release();
 
-   if (close(fd) < 0)
-   die_errno(_("closing file '%s'"), path);
-   return 0;
+   if (close(fd) < 0 && !res)
+   return error(_("closing file '%s': %s"), path, strerror(errno));
+
+   return res ? -1 : 0;
 }
 
 /*
@@ -4184,15 +4191,24 @@ static void create_one_file(struct apply_state *state,
const char *buf,
unsigned long size)
 {
+   int res;
+
if (state->cached)
return;
-   if (!try_create_file(path, mode, buf, size))
+
+   res = try_create_file(path, mode, buf, size);
+   if (res < 0)
+   exit(1);
+   if (!res)
return;
 
if (errno == ENOENT) {
if (safe_create_leading_directories(path))
return;
-   if (!try_create_file(path, mode, buf, size))
+   res = try_create_file(path, mode, buf, size);
+   if (res < 0)
+   exit(1);
+   if (!res)
return;
}
 
@@ -4211,7 +4227,10 @@ static void create_one_file(struct apply_state *state,
for (;;) {
char newpath[PATH_MAX];
mksnpath(newpath, sizeof(newpath), "%s~%u", path, nr);
-   if (!try_create_file(newpath, mode, buf, size)) {
+   res = try_create_file(newpath, mode, buf, size);
+   if (res < 0)
+   exit(1);
+   if (!res) {
if (!rename(newpath, path))
return;
unlink_or_warn(newpath);
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 26/44] builtin/apply: make create_one_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of exit()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", create_one_file() should return -1 instead of
calling exit().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 37f0c2e..f51dc60 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4184,32 +4184,36 @@ static int try_create_file(const char *path, unsigned 
int mode, const char *buf,
  * We optimistically assume that the directories exist,
  * which is true 99% of the time anyway. If they don't,
  * we create them and try again.
+ *
+ * Returns:
+ *   -1 on error
+ *   0 otherwise
  */
-static void create_one_file(struct apply_state *state,
-   char *path,
-   unsigned mode,
-   const char *buf,
-   unsigned long size)
+static int create_one_file(struct apply_state *state,
+  char *path,
+  unsigned mode,
+  const char *buf,
+  unsigned long size)
 {
int res;
 
if (state->cached)
-   return;
+   return 0;
 
res = try_create_file(path, mode, buf, size);
if (res < 0)
-   exit(1);
+   return -1;
if (!res)
-   return;
+   return 0;
 
if (errno == ENOENT) {
if (safe_create_leading_directories(path))
-   return;
+   return 0;
res = try_create_file(path, mode, buf, size);
if (res < 0)
-   exit(1);
+   return -1;
if (!res)
-   return;
+   return 0;
}
 
if (errno == EEXIST || errno == EACCES) {
@@ -4229,10 +4233,10 @@ static void create_one_file(struct apply_state *state,
mksnpath(newpath, sizeof(newpath), "%s~%u", path, nr);
res = try_create_file(newpath, mode, buf, size);
if (res < 0)
-   exit(1);
+   return -1;
if (!res) {
if (!rename(newpath, path))
-   return;
+   return 0;
unlink_or_warn(newpath);
break;
}
@@ -4241,7 +4245,8 @@ static void create_one_file(struct apply_state *state,
++nr;
}
}
-   die_errno(_("unable to write file '%s' mode %o"), path, mode);
+   return error(_("unable to write file '%s' mode %o: %s"),
+path, mode, strerror(errno));
 }
 
 static int add_conflicted_stages_file(struct apply_state *state,
@@ -4286,7 +4291,8 @@ static int create_file(struct apply_state *state, struct 
patch *patch)
 
if (!mode)
mode = S_IFREG | 0644;
-   create_one_file(state, path, mode, buf, size);
+   if (create_one_file(state, path, mode, buf, size))
+   return -1;
 
if (patch->conflicted_threeway)
return add_conflicted_stages_file(state, patch);
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 28/44] apply: rename and move opt constants to apply.h

[Christian Couder]

The constants for the "inaccurate-eof" and the "recount" options will
be used in both "apply.c" and "builtin/apply.c", so they need to go
into "apply.h", and therefore they need a name that is more specific
to the API they belong to.

Signed-off-by: Christian Couder 
---
 apply.h |  3 +++
 builtin/apply.c | 11 ---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/apply.h b/apply.h
index 1f2277e..3cc3da6 100644
--- a/apply.h
+++ b/apply.h
@@ -108,4 +108,7 @@ extern int init_apply_state(struct apply_state *state,
 extern void clear_apply_state(struct apply_state *state);
 extern int check_apply_state(struct apply_state *state, int force_apply);
 
+#define APPLY_OPT_INACCURATE_EOF   (1<<0)
+#define APPLY_OPT_RECOUNT  (1<<1)
+
 #endif
diff --git a/builtin/apply.c b/builtin/apply.c
index c84add0..a06ab55 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4449,9 +4449,6 @@ static int write_out_results(struct apply_state *state, 
struct patch *list)
 
 static struct lock_file lock_file;
 
-#define INACCURATE_EOF (1<<0)
-#define RECOUNT(1<<1)
-
 /*
  * Try to apply a patch.
  *
@@ -4480,8 +4477,8 @@ static int apply_patch(struct apply_state *state,
int nr;
 
patch = xcalloc(1, sizeof(*patch));
-   patch->inaccurate_eof = !!(options & INACCURATE_EOF);
-   patch->recount =  !!(options & RECOUNT);
+   patch->inaccurate_eof = !!(options & APPLY_OPT_INACCURATE_EOF);
+   patch->recount =  !!(options & APPLY_OPT_RECOUNT);
nr = parse_chunk(state, buf.buf + offset, buf.len - offset, 
patch);
if (nr < 0) {
free_patch(patch);
@@ -4785,10 +4782,10 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
OPT__VERBOSE(_verbosely, N_("be verbose")),
OPT_BIT(0, "inaccurate-eof", ,
N_("tolerate incorrectly detected missing new-line at 
the end of file"),
-   INACCURATE_EOF),
+   APPLY_OPT_INACCURATE_EOF),
OPT_BIT(0, "recount", ,
N_("do not trust the line counts in the hunk headers"),
-   RECOUNT),
+   APPLY_OPT_RECOUNT),
{ OPTION_CALLBACK, 0, "directory", , N_("root"),
N_("prepend  to all filenames"),
0, apply_option_parse_directory },
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 27/44] builtin/apply: rename option parsing functions

[Christian Couder]

As these functions are going to be part of the libified
apply api, let's give them a name that is more specific
to the apply api.

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index f51dc60..c84add0 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4566,16 +4566,16 @@ end:
return res;
 }
 
-static int option_parse_exclude(const struct option *opt,
-   const char *arg, int unset)
+static int apply_option_parse_exclude(const struct option *opt,
+ const char *arg, int unset)
 {
struct apply_state *state = opt->value;
add_name_limit(state, arg, 1);
return 0;
 }
 
-static int option_parse_include(const struct option *opt,
-   const char *arg, int unset)
+static int apply_option_parse_include(const struct option *opt,
+ const char *arg, int unset)
 {
struct apply_state *state = opt->value;
add_name_limit(state, arg, 0);
@@ -4583,9 +4583,9 @@ static int option_parse_include(const struct option *opt,
return 0;
 }
 
-static int option_parse_p(const struct option *opt,
- const char *arg,
- int unset)
+static int apply_option_parse_p(const struct option *opt,
+   const char *arg,
+   int unset)
 {
struct apply_state *state = opt->value;
state->p_value = atoi(arg);
@@ -4593,8 +4593,8 @@ static int option_parse_p(const struct option *opt,
return 0;
 }
 
-static int option_parse_space_change(const struct option *opt,
-const char *arg, int unset)
+static int apply_option_parse_space_change(const struct option *opt,
+  const char *arg, int unset)
 {
struct apply_state *state = opt->value;
if (unset)
@@ -4604,8 +4604,8 @@ static int option_parse_space_change(const struct option 
*opt,
return 0;
 }
 
-static int option_parse_whitespace(const struct option *opt,
-  const char *arg, int unset)
+static int apply_option_parse_whitespace(const struct option *opt,
+const char *arg, int unset)
 {
struct apply_state *state = opt->value;
state->whitespace_option = arg;
@@ -4614,8 +4614,8 @@ static int option_parse_whitespace(const struct option 
*opt,
return 0;
 }
 
-static int option_parse_directory(const struct option *opt,
- const char *arg, int unset)
+static int apply_option_parse_directory(const struct option *opt,
+   const char *arg, int unset)
 {
struct apply_state *state = opt->value;
strbuf_reset(>root);
@@ -4729,13 +4729,13 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
struct option builtin_apply_options[] = {
{ OPTION_CALLBACK, 0, "exclude", , N_("path"),
N_("don't apply changes matching the given path"),
-   0, option_parse_exclude },
+   0, apply_option_parse_exclude },
{ OPTION_CALLBACK, 0, "include", , N_("path"),
N_("apply changes matching the given path"),
-   0, option_parse_include },
+   0, apply_option_parse_include },
{ OPTION_CALLBACK, 'p', NULL, , N_("num"),
N_("remove  leading slashes from traditional diff 
paths"),
-   0, option_parse_p },
+   0, apply_option_parse_p },
OPT_BOOL(0, "no-add", _add,
N_("ignore additions made by the patch")),
OPT_BOOL(0, "stat", ,
@@ -4767,13 +4767,13 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
N_("ensure at least  lines of context 
match")),
{ OPTION_CALLBACK, 0, "whitespace", , N_("action"),
N_("detect new or modified lines that have whitespace 
errors"),
-   0, option_parse_whitespace },
+   0, apply_option_parse_whitespace },
{ OPTION_CALLBACK, 0, "ignore-space-change", , NULL,
N_("ignore changes in whitespace when finding context"),
-   PARSE_OPT_NOARG, option_parse_space_change },
+   PARSE_OPT_NOARG, apply_option_parse_space_change },
{ OPTION_CALLBACK, 0, "ignore-whitespace", , NULL,
N_("ignore changes in whitespace when finding context"),
-   PARSE_OPT_NOARG, option_parse_space_change },
+   

[PATCH v6 22/44] builtin/apply: make create_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of exit()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", create_file() should just return what
add_conflicted_stages_file() and add_index_file() are returning
instead of calling exit().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 005ba78..76d473c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4258,7 +4258,7 @@ static int add_conflicted_stages_file(struct apply_state 
*state,
return 0;
 }
 
-static void create_file(struct apply_state *state, struct patch *patch)
+static int create_file(struct apply_state *state, struct patch *patch)
 {
char *path = patch->new_name;
unsigned mode = patch->new_mode;
@@ -4269,13 +4269,10 @@ static void create_file(struct apply_state *state, 
struct patch *patch)
mode = S_IFREG | 0644;
create_one_file(state, path, mode, buf, size);
 
-   if (patch->conflicted_threeway) {
-   if (add_conflicted_stages_file(state, patch))
-   exit(1);
-   } else {
-   if (add_index_file(state, path, mode, buf, size))
-   exit(1);
-   }
+   if (patch->conflicted_threeway)
+   return add_conflicted_stages_file(state, patch);
+   else
+   return add_index_file(state, path, mode, buf, size);
 }
 
 /* phase zero is to remove, phase one is to create */
@@ -4291,8 +4288,10 @@ static void write_out_one_result(struct apply_state 
*state,
return;
}
if (patch->is_new > 0 || patch->is_copy) {
-   if (phase == 1)
-   create_file(state, patch);
+   if (phase == 1) {
+   if (create_file(state, patch))
+   exit(1);
+   }
return;
}
/*
@@ -4303,8 +4302,10 @@ static void write_out_one_result(struct apply_state 
*state,
if (remove_file(state, patch, patch->is_rename))
exit(1);
}
-   if (phase == 1)
-   create_file(state, patch);
+   if (phase == 1) {
+   if (create_file(state, patch))
+   exit(1);
+   }
 }
 
 static int write_out_one_reject(struct apply_state *state, struct patch *patch)
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 21/44] builtin/apply: make add_index_file() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", add_index_file() should return -1 instead of
calling die().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 48 +++-
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 0997384..005ba78 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4088,11 +4088,11 @@ static int remove_file(struct apply_state *state, 
struct patch *patch, int rmdir
return 0;
 }
 
-static void add_index_file(struct apply_state *state,
-  const char *path,
-  unsigned mode,
-  void *buf,
-  unsigned long size)
+static int add_index_file(struct apply_state *state,
+ const char *path,
+ unsigned mode,
+ void *buf,
+ unsigned long size)
 {
struct stat st;
struct cache_entry *ce;
@@ -4100,7 +4100,7 @@ static void add_index_file(struct apply_state *state,
unsigned ce_size = cache_entry_size(namelen);
 
if (!state->update_index)
-   return;
+   return 0;
 
ce = xcalloc(1, ce_size);
memcpy(ce->name, path, namelen);
@@ -4111,20 +4111,32 @@ static void add_index_file(struct apply_state *state,
const char *s;
 
if (!skip_prefix(buf, "Subproject commit ", ) ||
-   get_sha1_hex(s, ce->sha1))
-   die(_("corrupt patch for submodule %s"), path);
+   get_sha1_hex(s, ce->sha1)) {
+   free(ce);
+   return error(_("corrupt patch for submodule %s"), path);
+   }
} else {
if (!state->cached) {
-   if (lstat(path, ) < 0)
-   die_errno(_("unable to stat newly created file 
'%s'"),
- path);
+   if (lstat(path, ) < 0) {
+   free(ce);
+   return error(_("unable to stat newly "
+  "created file '%s': %s"),
+path, strerror(errno));
+   }
fill_stat_cache_info(ce, );
}
-   if (write_sha1_file(buf, size, blob_type, ce->sha1) < 0)
-   die(_("unable to create backing store for newly created 
file %s"), path);
+   if (write_sha1_file(buf, size, blob_type, ce->sha1) < 0) {
+   free(ce);
+   return error(_("unable to create backing store "
+  "for newly created file %s"), path);
+   }
}
-   if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0)
-   die(_("unable to add cache entry for %s"), path);
+   if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD) < 0) {
+   free(ce);
+   return error(_("unable to add cache entry for %s"), path);
+   }
+
+   return 0;
 }
 
 static int try_create_file(const char *path, unsigned int mode, const char 
*buf, unsigned long size)
@@ -4260,8 +4272,10 @@ static void create_file(struct apply_state *state, 
struct patch *patch)
if (patch->conflicted_threeway) {
if (add_conflicted_stages_file(state, patch))
exit(1);
-   } else
-   add_index_file(state, path, mode, buf, size);
+   } else {
+   if (add_index_file(state, path, mode, buf, size))
+   exit(1);
+   }
 }
 
 /* phase zero is to remove, phase one is to create */
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 07/44] builtin/apply: make parse_whitespace_option() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in builtin/apply.c, parse_whitespace_option() should return -1 instead
of calling die().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 2671586..e56e754 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -27,34 +27,34 @@ static const char * const apply_usage[] = {
NULL
 };
 
-static void parse_whitespace_option(struct apply_state *state, const char 
*option)
+static int parse_whitespace_option(struct apply_state *state, const char 
*option)
 {
if (!option) {
state->ws_error_action = warn_on_ws_error;
-   return;
+   return 0;
}
if (!strcmp(option, "warn")) {
state->ws_error_action = warn_on_ws_error;
-   return;
+   return 0;
}
if (!strcmp(option, "nowarn")) {
state->ws_error_action = nowarn_ws_error;
-   return;
+   return 0;
}
if (!strcmp(option, "error")) {
state->ws_error_action = die_on_ws_error;
-   return;
+   return 0;
}
if (!strcmp(option, "error-all")) {
state->ws_error_action = die_on_ws_error;
state->squelch_whitespace_errors = 0;
-   return;
+   return 0;
}
if (!strcmp(option, "strip") || !strcmp(option, "fix")) {
state->ws_error_action = correct_ws_error;
-   return;
+   return 0;
}
-   die(_("unrecognized whitespace option '%s'"), option);
+   return error(_("unrecognized whitespace option '%s'"), option);
 }
 
 static void parse_ignorewhitespace_option(struct apply_state *state,
@@ -4579,7 +4579,8 @@ static int option_parse_whitespace(const struct option 
*opt,
 {
struct apply_state *state = opt->value;
state->whitespace_option = arg;
-   parse_whitespace_option(state, arg);
+   if (parse_whitespace_option(state, arg))
+   exit(1);
return 0;
 }
 
@@ -4613,8 +4614,8 @@ static void init_apply_state(struct apply_state *state,
strbuf_init(>root, 0);
 
git_apply_config();
-   if (apply_default_whitespace)
-   parse_whitespace_option(state, apply_default_whitespace);
+   if (apply_default_whitespace && parse_whitespace_option(state, 
apply_default_whitespace))
+   exit(1);
if (apply_default_ignorewhitespace)
parse_ignorewhitespace_option(state, 
apply_default_ignorewhitespace);
 }
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 04/44] builtin/apply: make find_header() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in builtin/apply.c, find_header() should return -1 instead of calling
die().

Unfortunately find_header() already returns -1 when no header is found,
so let's make it return -2 instead in this case.

Signed-off-by: Christian Couder 
---
 builtin/apply.c   | 33 ++---
 t/t4254-am-corrupt.sh |  2 +-
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 2ff8450..630c01c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1419,6 +1419,14 @@ static int parse_fragment_header(const char *line, int 
len, struct fragment *fra
return offset;
 }
 
+/*
+ * Find file diff header
+ *
+ * Returns:
+ *  -1 in case of error
+ *  -2 if no header was found
+ *   the size of the header in bytes (called "offset") otherwise
+ */
 static int find_header(struct apply_state *state,
   const char *line,
   unsigned long size,
@@ -1452,8 +1460,8 @@ static int find_header(struct apply_state *state,
struct fragment dummy;
if (parse_fragment_header(line, len, ) < 0)
continue;
-   die(_("patch fragment without header at line %d: %.*s"),
-   state->linenr, (int)len-1, line);
+   return error(_("patch fragment without header at line 
%d: %.*s"),
+state->linenr, (int)len-1, line);
}
 
if (size < len + 6)
@@ -1469,18 +1477,18 @@ static int find_header(struct apply_state *state,
continue;
if (!patch->old_name && !patch->new_name) {
if (!patch->def_name)
-   die(Q_("git diff header lacks filename 
information when removing "
-  "%d leading pathname component 
(line %d)",
-  "git diff header lacks filename 
information when removing "
-  "%d leading pathname components 
(line %d)",
-  state->p_value),
-   state->p_value, state->linenr);
+   return error(Q_("git diff header lacks 
filename information when removing "
+   "%d leading pathname 
component (line %d)",
+   "git diff header lacks 
filename information when removing "
+   "%d leading pathname 
components (line %d)",
+   state->p_value),
+state->p_value, 
state->linenr);
patch->old_name = xstrdup(patch->def_name);
patch->new_name = xstrdup(patch->def_name);
}
if (!patch->is_delete && !patch->new_name)
-   die("git diff header lacks filename information 
"
-   "(line %d)", state->linenr);
+   return error("git diff header lacks filename 
information "
+"(line %d)", state->linenr);
patch->is_toplevel_relative = 1;
*hdrsize = git_hdr_len;
return offset;
@@ -1505,7 +1513,7 @@ static int find_header(struct apply_state *state,
state->linenr += 2;
return offset;
}
-   return -1;
+   return -2;
 }
 
 static void record_ws_error(struct apply_state *state,
@@ -1996,6 +2004,9 @@ static int parse_chunk(struct apply_state *state, char 
*buffer, unsigned long si
int hdrsize, patchsize;
int offset = find_header(state, buffer, size, , patch);
 
+   if (offset == -1)
+   exit(1);
+
if (offset < 0)
return offset;
 
diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
index 85716dd..9bd7dd2 100755
--- a/t/t4254-am-corrupt.sh
+++ b/t/t4254-am-corrupt.sh
@@ -29,7 +29,7 @@ test_expect_success 'try to apply corrupted patch' '
 '
 
 test_expect_success 'compare diagnostic; ensure file is still here' '
-   echo "fatal: git diff header lacks filename information (line 4)" 
>expected &&
+   echo "error: git diff header lacks filename information (line 4)" 
>expected &&
test_path_is_file f &&
test_cmp expected actual
 '
-- 
2.9.0.rc2.362.g3cd93d0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the 

[PATCH v6 10/44] apply: make init_apply_state() return -1 instead of exit()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of exit()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", init_apply_state() should return -1 instead of
calling exit().

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 apply.c | 11 ++-
 apply.h |  6 +++---
 builtin/apply.c |  3 ++-
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/apply.c b/apply.c
index 1f31bb4..c5a9ee2 100644
--- a/apply.c
+++ b/apply.c
@@ -55,9 +55,9 @@ int parse_ignorewhitespace_option(struct apply_state *state,
return error(_("unrecognized whitespace ignore option '%s'"), option);
 }
 
-void init_apply_state(struct apply_state *state,
- const char *prefix,
- struct lock_file *lock_file)
+int init_apply_state(struct apply_state *state,
+const char *prefix,
+struct lock_file *lock_file)
 {
memset(state, 0, sizeof(*state));
state->prefix = prefix;
@@ -76,9 +76,10 @@ void init_apply_state(struct apply_state *state,
 
git_apply_config();
if (apply_default_whitespace && parse_whitespace_option(state, 
apply_default_whitespace))
-   exit(1);
+   return -1;
if (apply_default_ignorewhitespace && 
parse_ignorewhitespace_option(state, apply_default_ignorewhitespace))
-   exit(1);
+   return -1;
+   return 0;
 }
 
 void clear_apply_state(struct apply_state *state)
diff --git a/apply.h b/apply.h
index 77502be..7d3a03b 100644
--- a/apply.h
+++ b/apply.h
@@ -102,9 +102,9 @@ extern int parse_whitespace_option(struct apply_state 
*state,
 extern int parse_ignorewhitespace_option(struct apply_state *state,
 const char *option);
 
-extern void init_apply_state(struct apply_state *state,
-const char *prefix,
-struct lock_file *lock_file);
+extern int init_apply_state(struct apply_state *state,
+   const char *prefix,
+   struct lock_file *lock_file);
 extern void clear_apply_state(struct apply_state *state);
 
 #endif
diff --git a/builtin/apply.c b/builtin/apply.c
index bc15545..2ae1243 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4728,7 +4728,8 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   init_apply_state(, prefix, _file);
+   if (init_apply_state(, prefix, _file))
+   exit(1);
 
argc = parse_options(argc, argv, state.prefix, builtin_apply_options,
apply_usage, 0);
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 17/44] builtin/apply: change die_on_unsafe_path() to check_unsafe_path()

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", die_on_unsafe_path() should return -1 using
error() instead of calling die(), so while doing that let's change
its name to check_unsafe_path().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index b506369..429fddd 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3697,7 +3697,7 @@ static int path_is_beyond_symlink(struct apply_state 
*state, const char *name_)
return ret;
 }
 
-static void die_on_unsafe_path(struct patch *patch)
+static int check_unsafe_path(struct patch *patch)
 {
const char *old_name = NULL;
const char *new_name = NULL;
@@ -3709,9 +3709,10 @@ static void die_on_unsafe_path(struct patch *patch)
new_name = patch->new_name;
 
if (old_name && !verify_path(old_name))
-   die(_("invalid path '%s'"), old_name);
+   return error(_("invalid path '%s'"), old_name);
if (new_name && !verify_path(new_name))
-   die(_("invalid path '%s'"), new_name);
+   return error(_("invalid path '%s'"), new_name);
+   return 0;
 }
 
 /*
@@ -3801,8 +3802,8 @@ static int check_patch(struct apply_state *state, struct 
patch *patch)
}
}
 
-   if (!state->unsafe_paths)
-   die_on_unsafe_path(patch);
+   if (!state->unsafe_paths && check_unsafe_path(patch))
+   return -1;
 
/*
 * An attempt to read from or delete a path that is beyond a
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 23/44] builtin/apply: make write_out_one_result() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of exit()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", write_out_one_result() should just return what
remove_file() and create_file() are returning instead of calling
exit().

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 38 --
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 76d473c..291e24e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4276,36 +4276,29 @@ static int create_file(struct apply_state *state, 
struct patch *patch)
 }
 
 /* phase zero is to remove, phase one is to create */
-static void write_out_one_result(struct apply_state *state,
-struct patch *patch,
-int phase)
+static int write_out_one_result(struct apply_state *state,
+   struct patch *patch,
+   int phase)
 {
if (patch->is_delete > 0) {
-   if (phase == 0) {
-   if (remove_file(state, patch, 1))
-   exit(1);
-   }
-   return;
+   if (phase == 0)
+   return remove_file(state, patch, 1);
+   return 0;
}
if (patch->is_new > 0 || patch->is_copy) {
-   if (phase == 1) {
-   if (create_file(state, patch))
-   exit(1);
-   }
-   return;
+   if (phase == 1)
+   return create_file(state, patch);
+   return 0;
}
/*
 * Rename or modification boils down to the same
 * thing: remove the old, write the new
 */
-   if (phase == 0) {
-   if (remove_file(state, patch, patch->is_rename))
-   exit(1);
-   }
-   if (phase == 1) {
-   if (create_file(state, patch))
-   exit(1);
-   }
+   if (phase == 0)
+   return remove_file(state, patch, patch->is_rename);
+   if (phase == 1)
+   return create_file(state, patch);
+   return 0;
 }
 
 static int write_out_one_reject(struct apply_state *state, struct patch *patch)
@@ -4392,7 +4385,8 @@ static int write_out_results(struct apply_state *state, 
struct patch *list)
if (l->rejected)
errs = 1;
else {
-   write_out_one_result(state, l, phase);
+   if (write_out_one_result(state, l, phase))
+   exit(1);
if (phase == 1) {
if (write_out_one_reject(state, l))
errs = 1;
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 16/44] builtin/apply: make gitdiff_*() return -1 on error

[Christian Couder]

To libify `git apply` functionality we have to signal errors to the
caller instead of die()ing.

To do that in a compatible manner with the rest of the error handling
in "builtin/apply.c", gitdiff_*() functions should return -1 instead
of calling die().

A previous patch made it possible for gitdiff_*() functions to
return -1 in case of error. Let's take advantage of that to
make gitdiff_verify_name() return -1 on error, and to have
gitdiff_oldname() and gitdiff_newname() directly return
what gitdiff_verify_name() returns.

Helped-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 40 +---
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 1142514..b506369 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -827,54 +827,56 @@ static int gitdiff_hdrend(struct apply_state *state,
 #define DIFF_OLD_NAME 0
 #define DIFF_NEW_NAME 1
 
-static void gitdiff_verify_name(struct apply_state *state,
-   const char *line,
-   int isnull,
-   char **name,
-   int side)
+static int gitdiff_verify_name(struct apply_state *state,
+  const char *line,
+  int isnull,
+  char **name,
+  int side)
 {
if (!*name && !isnull) {
*name = find_name(state, line, NULL, state->p_value, TERM_TAB);
-   return;
+   return 0;
}
 
if (*name) {
int len = strlen(*name);
char *another;
if (isnull)
-   die(_("git apply: bad git-diff - expected /dev/null, 
got %s on line %d"),
-   *name, state->linenr);
+   return error(_("git apply: bad git-diff - expected 
/dev/null, got %s on line %d"),
+*name, state->linenr);
another = find_name(state, line, NULL, state->p_value, 
TERM_TAB);
-   if (!another || memcmp(another, *name, len + 1))
-   die((side == DIFF_NEW_NAME) ?
+   if (!another || memcmp(another, *name, len + 1)) {
+   free(another);
+   return error((side == DIFF_NEW_NAME) ?
_("git apply: bad git-diff - inconsistent new 
filename on line %d") :
_("git apply: bad git-diff - inconsistent old 
filename on line %d"), state->linenr);
+   }
free(another);
} else {
/* expect "/dev/null" */
if (memcmp("/dev/null", line, 9) || line[9] != '\n')
-   die(_("git apply: bad git-diff - expected /dev/null on 
line %d"), state->linenr);
+   return error(_("git apply: bad git-diff - expected 
/dev/null on line %d"), state->linenr);
}
+
+   return 0;
 }
 
 static int gitdiff_oldname(struct apply_state *state,
   const char *line,
   struct patch *patch)
 {
-   gitdiff_verify_name(state, line,
-   patch->is_new, >old_name,
-   DIFF_OLD_NAME);
-   return 0;
+   return gitdiff_verify_name(state, line,
+  patch->is_new, >old_name,
+  DIFF_OLD_NAME);
 }
 
 static int gitdiff_newname(struct apply_state *state,
   const char *line,
   struct patch *patch)
 {
-   gitdiff_verify_name(state, line,
-   patch->is_delete, >new_name,
-   DIFF_NEW_NAME);
-   return 0;
+   return gitdiff_verify_name(state, line,
+  patch->is_delete, >new_name,
+  DIFF_NEW_NAME);
 }
 
 static int gitdiff_oldmode(struct apply_state *state,
-- 
2.9.0.rc2.362.g3cd93d0

--
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 v6 01/44] apply: move 'struct apply_state' to apply.h

[Christian Couder]

To libify `git apply` functionality we must make 'struct apply_state'
usable outside "builtin/apply.c".

Let's do that by creating a new "apply.h" and moving
'struct apply_state' there.

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 apply.h | 100 
 builtin/apply.c |  98 +-
 2 files changed, 101 insertions(+), 97 deletions(-)
 create mode 100644 apply.h

diff --git a/apply.h b/apply.h
new file mode 100644
index 000..9a98eae
--- /dev/null
+++ b/apply.h
@@ -0,0 +1,100 @@
+#ifndef APPLY_H
+#define APPLY_H
+
+enum ws_error_action {
+   nowarn_ws_error,
+   warn_on_ws_error,
+   die_on_ws_error,
+   correct_ws_error
+};
+
+enum ws_ignore {
+   ignore_ws_none,
+   ignore_ws_change
+};
+
+/*
+ * We need to keep track of how symlinks in the preimage are
+ * manipulated by the patches.  A patch to add a/b/c where a/b
+ * is a symlink should not be allowed to affect the directory
+ * the symlink points at, but if the same patch removes a/b,
+ * it is perfectly fine, as the patch removes a/b to make room
+ * to create a directory a/b so that a/b/c can be created.
+ *
+ * See also "struct string_list symlink_changes" in "struct
+ * apply_state".
+ */
+#define SYMLINK_GOES_AWAY 01
+#define SYMLINK_IN_RESULT 02
+
+struct apply_state {
+   const char *prefix;
+   int prefix_length;
+
+   /* These are lock_file related */
+   struct lock_file *lock_file;
+   int newfd;
+
+   /* These control what gets looked at and modified */
+   int apply; /* this is not a dry-run */
+   int cached; /* apply to the index only */
+   int check; /* preimage must match working tree, don't actually apply */
+   int check_index; /* preimage must match the indexed version */
+   int update_index; /* check_index && apply */
+
+   /* These control cosmetic aspect of the output */
+   int diffstat; /* just show a diffstat, and don't actually apply */
+   int numstat; /* just show a numeric diffstat, and don't actually apply 
*/
+   int summary; /* just report creation, deletion, etc, and don't actually 
apply */
+
+   /* These boolean parameters control how the apply is done */
+   int allow_overlap;
+   int apply_in_reverse;
+   int apply_with_reject;
+   int apply_verbosely;
+   int no_add;
+   int threeway;
+   int unidiff_zero;
+   int unsafe_paths;
+
+   /* Other non boolean parameters */
+   const char *fake_ancestor;
+   const char *patch_input_file;
+   int line_termination;
+   struct strbuf root;
+   int p_value;
+   int p_value_known;
+   unsigned int p_context;
+
+   /* Exclude and include path parameters */
+   struct string_list limit_by_name;
+   int has_include;
+
+   /* Various "current state" */
+   int linenr; /* current line number */
+   struct string_list symlink_changes; /* we have to track symlinks */
+
+   /*
+* For "diff-stat" like behaviour, we keep track of the biggest change
+* we've seen, and the longest filename. That allows us to do simple
+* scaling.
+*/
+   int max_change;
+   int max_len;
+
+   /*
+* Records filenames that have been touched, in order to handle
+* the case where more than one patches touch the same file.
+*/
+   struct string_list fn_table;
+
+   /* These control whitespace errors */
+   enum ws_error_action ws_error_action;
+   enum ws_ignore ws_ignore_action;
+   const char *whitespace_option;
+   int whitespace_error;
+   int squelch_whitespace_errors;
+   int applied_after_fixing_ws;
+};
+
+#endif
diff --git a/builtin/apply.c b/builtin/apply.c
index ecb7f1b..3a0c53a 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -20,103 +20,7 @@
 #include "xdiff-interface.h"
 #include "ll-merge.h"
 #include "rerere.h"
-
-enum ws_error_action {
-   nowarn_ws_error,
-   warn_on_ws_error,
-   die_on_ws_error,
-   correct_ws_error
-};
-
-
-enum ws_ignore {
-   ignore_ws_none,
-   ignore_ws_change
-};
-
-/*
- * We need to keep track of how symlinks in the preimage are
- * manipulated by the patches.  A patch to add a/b/c where a/b
- * is a symlink should not be allowed to affect the directory
- * the symlink points at, but if the same patch removes a/b,
- * it is perfectly fine, as the patch removes a/b to make room
- * to create a directory a/b so that a/b/c can be created.
- *
- * See also "struct string_list symlink_changes" in "struct
- * apply_state".
- */
-#define SYMLINK_GOES_AWAY 01
-#define SYMLINK_IN_RESULT 02
-
-struct apply_state {
-   const char *prefix;
-   int prefix_length;
-
-   /* These are lock_file related */
-   struct lock_file *lock_file;
-   int newfd;
-
-   /* These control 

[PATCH v6 02/44] builtin/apply: make apply_patch() return -1 instead of die()ing

[Christian Couder]

To libify `git apply` functionality we have to signal errors
to the caller instead of die()ing.

As a first step in this direction, let's make apply_patch() return
-1 in case of errors instead of dying. For now its only caller
apply_all_patches() will exit(1) when apply_patch() return -1.

In a later patch, apply_all_patches() will return -1 too instead of
exiting.

Helped-by: Eric Sunshine 
Signed-off-by: Christian Couder 
---
 builtin/apply.c | 54 +++---
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 3a0c53a..598e479 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4404,6 +4404,14 @@ static struct lock_file lock_file;
 #define INACCURATE_EOF (1<<0)
 #define RECOUNT(1<<1)
 
+/*
+ * Try to apply a patch.
+ *
+ * Returns:
+ *  -1 if an error happened
+ *   0 if the patch applied
+ *   1 if the patch did not apply
+ */
 static int apply_patch(struct apply_state *state,
   int fd,
   const char *filename,
@@ -4413,6 +4421,7 @@ static int apply_patch(struct apply_state *state,
struct strbuf buf = STRBUF_INIT; /* owns the patch text */
struct patch *list = NULL, **listp = 
int skipped_patch = 0;
+   int res = 0;
 
state->patch_input_file = filename;
read_patch_file(, fd);
@@ -4445,8 +4454,10 @@ static int apply_patch(struct apply_state *state,
offset += nr;
}
 
-   if (!list && !skipped_patch)
-   die(_("unrecognized input"));
+   if (!list && !skipped_patch) {
+   res = error(_("unrecognized input"));
+   goto end;
+   }
 
if (state->whitespace_error && (state->ws_error_action == 
die_on_ws_error))
state->apply = 0;
@@ -4455,21 +4466,22 @@ static int apply_patch(struct apply_state *state,
if (state->update_index && state->newfd < 0)
state->newfd = hold_locked_index(state->lock_file, 1);
 
-   if (state->check_index) {
-   if (read_cache() < 0)
-   die(_("unable to read index file"));
+   if (state->check_index && read_cache() < 0) {
+   res = error(_("unable to read index file"));
+   goto end;
}
 
if ((state->check || state->apply) &&
check_patch_list(state, list) < 0 &&
-   !state->apply_with_reject)
-   exit(1);
+   !state->apply_with_reject) {
+   res = -1;
+   goto end;
+   }
 
if (state->apply && write_out_results(state, list)) {
-   if (state->apply_with_reject)
-   exit(1);
/* with --3way, we still need to write the index out */
-   return 1;
+   res = state->apply_with_reject ? -1 : 1;
+   goto end;
}
 
if (state->fake_ancestor)
@@ -4484,10 +4496,11 @@ static int apply_patch(struct apply_state *state,
if (state->summary)
summary_patch_list(list);
 
+end:
free_patch_list(list);
strbuf_release();
string_list_clear(>fn_table, 0);
-   return 0;
+   return res;
 }
 
 static void git_apply_config(void)
@@ -4625,6 +4638,7 @@ static int apply_all_patches(struct apply_state *state,
 int options)
 {
int i;
+   int res;
int errs = 0;
int read_stdin = 1;
 
@@ -4633,7 +4647,10 @@ static int apply_all_patches(struct apply_state *state,
int fd;
 
if (!strcmp(arg, "-")) {
-   errs |= apply_patch(state, 0, "", options);
+   res = apply_patch(state, 0, "", options);
+   if (res < 0)
+   exit(1);
+   errs |= res;
read_stdin = 0;
continue;
} else if (0 < state->prefix_length)
@@ -4646,12 +4663,19 @@ static int apply_all_patches(struct apply_state *state,
die_errno(_("can't open patch '%s'"), arg);
read_stdin = 0;
set_default_whitespace_mode(state);
-   errs |= apply_patch(state, fd, arg, options);
+   res = apply_patch(state, fd, arg, options);
+   if (res < 0)
+   exit(1);
+   errs |= res;
close(fd);
}
set_default_whitespace_mode(state);
-   if (read_stdin)
-   errs |= apply_patch(state, 0, "", options);
+   if (read_stdin) {
+   res = apply_patch(state, 0, "", options);
+   if (res < 0)
+   exit(1);
+   errs |= res;
+   }
 
if (state->whitespace_error) {
if (state->squelch_whitespace_errors &&
-- 
2.9.0.rc2.362.g3cd93d0

--
To 

[PATCH v6 00/44] libify apply and use lib in am, part 2

[Christian Couder]

Goal


This is a patch series about libifying `git apply` functionality, and
using this libified functionality in `git am`, so that no 'git apply'
process is spawn anymore. This makes `git am` significantly faster, so
`git rebase`, when it uses the am backend, is also significantly
faster.

Previous discussions and patches series
~~~

This has initially been discussed in the following thread:

  http://thread.gmane.org/gmane.comp.version-control.git/287236/

Then the following patch series were sent:

RFC: http://thread.gmane.org/gmane.comp.version-control.git/288489/
v1: http://thread.gmane.org/gmane.comp.version-control.git/292324/
v2: http://thread.gmane.org/gmane.comp.version-control.git/294248/
v3: http://thread.gmane.org/gmane.comp.version-control.git/295429/
v4: http://thread.gmane.org/gmane.comp.version-control.git/296350/
v5: http://thread.gmane.org/gmane.comp.version-control.git/296490/

Highlevel view of the patches in the series
~~~

This new patch series is built on top of the above previous work.

More precisely, this is "part 2" of the full patch series which is
built on top of the "part 1" of the full patch series. And as the
"part 1" is now in "next", this "part 2" is built on top of "next".

  - Patches 01/44 to 33/44 were in v1 and v2.

They finish libifying the apply functionality that was in
builtin/apply.c and move it into apply.{c,h}. And they use this
libified functionality in git am so that it doesn't launch git apply
processes any more.

Following great suggestions from Eric and Junio, there are some
changes in these patches to improve on v2:

  - Many commit messages for patches that make a function return
-1 were simplified by removing "by using error()".

  - 'struct lockfile' instance should be managed properly, as
rollback_lock_file() should be called in all error paths.

  - The patch that added calls to rollback_lock_file() has been
squashed into the patch that make apply_all_patches() return
-1 on error. The resulting patch is 13/44.

  - 'struct apply_state' is now moved to apply.h at the beginning
of this series.

  - Some useless braces were removed and the commit message was
fixed in patch 05/44.

  - error_errno() is now used instead of error() in patch 03/44.

  - Patches 34/44 to 43/44 were new in v2.

They implement a way to make the libified apply silent. It is a new
feature in the libified apply functionality.

This could be in a separate series, but unfortunately using the
libified apply in "git am" unmasks the fact that "git am", since it
was a shell script, has been silencing the apply functionality by
redirecting file descriptors to /dev/null and it looks like this is
not acceptable in C.

I am not yet sure that "be_silent" is a good name for the new
variable added by these patches.

Path 43/44 that adds --silent to `git apply` should probably be
discarded. I plan to do it in the next version.

I had planned to perhaps add tests for this new feature, but if 43/44
is discarded it may not be needed anymore.

  - Patch 44/44 is new.

It replaces some calls to error() with calls to error_errno().

General comments


Sorry if this patch series is still long. I can split it into two or
more series if it is prefered.

I can also send diffs between this version and the previous one, but
for now I'd rather not send them in this email, as it would make it
very long.

The benefits are not just related to not creating new processes. When
`git am` launched a `git apply` process, this new process had to read
the index from disk. Then after the `git apply`process had terminated,
`git am` dropped its index and read the index from disk to get the
index that had been modified by the `git apply`process. This was
inefficient and also prevented the split-index mechanism to provide
many performance benefits.

Using this series as rebase material, Duy explains it like this:

 > Without the series, the picture is not so surprising. We run git-apply
 > 80+ times, each consists of this sequence
 >
 > read index
 > write index (cache tree updates only)
 > read index again
 > optionally initialize name hash (when new entries are added, I guess)
 > read packed-refs
 > write index
 >
 > With this series, we run a single git-apply which does
 >
 > read index (and sharedindex too if in split-index mode)
 > initialize name hash
 > write index 80+ times

(See: 
http://thread.gmane.org/gmane.comp.version-control.git/292324/focus=292460)

Links
~

This patch series is available here:

https://github.com/chriscool/git/commits/libify-apply-use-in-am65

The previous versions are available there:

v1: https://github.com/chriscool/git/commits/libify-apply-use-in-am25 
v2: https://github.com/chriscool/git/commits/libify-apply-use-in-am54

Performance numbers
~~~

Only tests on Linux have been performed using a very 

Re: [PATCH 06/27] upload-pack: move "unshallow" sending code out of deepen()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> +static void deepen(int depth, const struct object_array *shallows)
> +{
> + struct commit_list *result = NULL;
> + int i;
> + if (depth == INFINITE_DEPTH && !is_repository_shallow())
> + for (i = 0; i < shallows->nr; i++) {
> + struct object *object = shallows->objects[i].item;
> + object->flags |= NOT_SHALLOW;
> + }
> + else
> + result = get_shallow_commits(_obj, depth,
> +  SHALLOW, NOT_SHALLOW);
> + send_shallow(result);
> + free_commit_list(result);
> + send_unshallow(shallows);
>   packet_flush(1);
>  }

Starting from the original up to this point, the code structure of
this function have bothered me quite a bit because I would expect

if (depth == INFINITE_DEPTH && !is_repository_shallow()) {
for (i = 0; i < shallows->nr; i++) {
struct object *object = shallows->objects[i].item;
object->flags |= NOT_SHALLOW;
}
} else {
struct commit_list *result;
result = get_shallow_commits(_obj, depth,
 SHALLOW, NOT_SHALLOW);
send_shallow(result);
free_commit_list(result);
}
send_unshallow(shallows);
packet_flush(1);

would be easier to understand.  The function seems to be reshaped
further in the series, so let's keep reading...
--
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 05/27] upload-pack: remove unused variable "backup"

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> After the last patch, "result" and "backup" are the same. "result" used
> to move, but the movement is now contained in send_shallow(). Delete
> this redundant variable.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> Signed-off-by: Junio C Hamano 
> ---

Ah, OK ;-)  Having this as a separate step is OK, too.
--
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 04/27] upload-pack: move "shallow" sending code out of deepen()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> @@ -551,16 +565,7 @@ static void deepen(int depth, const struct object_array 
> *shallows)
>   backup = result =
>   get_shallow_commits(_obj, depth,
>   SHALLOW, NOT_SHALLOW);
> - while (result) {
> - struct object *object = >item->object;
> - if (!(object->flags & (CLIENT_SHALLOW|NOT_SHALLOW))) {
> - packet_write(1, "shallow %s",
> -  oid_to_hex(>oid));
> - register_shallow(object->oid.hash);
> - shallow_nr++;
> - }
> - result = result->next;
> - }
> + send_shallow(result);
>   free_commit_list(backup);

At this point in the series, "backup" becomes an unnecessary and
redundant variable, as you can free_commit_list(result) here, right?

>   for (i = 0; i < shallows->nr; i++) {
>   struct object *object = shallows->objects[i].item;
--
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 1/2] bisect--helper: `is_expected_rev` shell function in C

[Eric Sunshine]

On Fri, Jun 10, 2016 at 9:39 AM, Pranit Bauva  wrote:
> On Fri, Jun 10, 2016 at 3:03 AM, Eric Sunshine  
> wrote:
>> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva  wrote:
>>> Reimplement `is_expected_rev` shell function in C. This will further be
>>> called from `check_expected_revs` function. This is a quite small
>>> function thus subcommand facility is redundant.
>>
>> This patch should be squashed into patch 2/2, as it is otherwise
>> pointless without that patch, and merely adds dead code.
>
> Sure I will squash and will explain it in the commit message.

Explain what in the commit message? If anything, I'd expect the commit
message to shrink since you won't need to explain anymore that this
function is split out.

>>> +   if (!file_exists(git_path_bisect_expected_rev()))
>>> +   return 0;
>>
>> Invoking file_exists() seems unnecessarily redundant when you can
>> discern effectively the same by checking the return value of
>> strbuf_read_file() below. I'd drop the file_exists() check altogether.
>
> I wanted to imitate the code. But I guess it would actually be better
> if I drop this file_exists().

There is a bit of a lesson to be learned by this example. While it's
true that the C conversion should retain the behavior of the original
shell code, that does not mean blindly mirroring the implementation
line for line is a good idea. A couple things to take into
consideration:

There are idiomatic ways of doing things in each language. What is
idiomatic in shell is not necessarily so in C. The C conversion should
employ C idioms and flow in a way which is natural for C code.

Consider what the original shell code is doing at a higher level than
merely by reading it line-by-line. In the case in question, the code
is:

test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")

While it's true that it's asking "does the file exist and is its value
the same as $1", the 'test -f' avoids a "file not found" error from
the $(cat ...) invocation. Since the return value of
strbuf_read_file() effectively encapsulates the "does the file exist"
check, a separate check isn't really needed.

>>> +   if (!strbuf_read_file(_hex, git_path_bisect_expected_rev(), 
>>> 0))
>>> +   return 0;
>>
>> What exactly is this trying to do? Considering that strbuf_read_file()
>> returns -1 upon error, otherwise the number of bytes read, if I'm
>> reading this correctly, is_expected_rev() returns false if
>> strbuf_read_file() encounters an error (which is fine) but also when
>> it successfully reads the file and its content length is non-zero
>> (which is very odd).
>>
>>> +   strbuf_trim(_hex);
>>> +   return !strcmp(actual_hex.buf, expected_hex);
>>
>> Thus, it only ever gets to this point if the file exists but is empty,
>> which is very unlikely to match 'expected_hex'. I could understand it
>> if you checked the result of strbuf_read_file() with <0 or even <=0,
>> but the current code doesn't make sense to me.
>>
>> Am I misunderstanding?
>
> Definitely not. Thanks for pointing it out. :) It went off my head
> that strbuf_read_file returns the bytes it reads. Also the code
> comment regarding strbuf_read_file does not mention it which probably
> misguided me. I should also send a fixing patch so that someone else
> does not fall into this like I did.

Out of curiosity, did the test suite pass with this patch applied?
This is such an egregious bug that it's hard to imagine the tests
passing, but if they did, then that may be a good indication that
coverage is too sparse and ought to be improved.
--
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] Fix spelling of "occurred"

[Peter Colberg]

Signed-off-by: Peter Colberg 
---
 config.c | 2 +-
 refs.h   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index f51c56b..d7ce34b 100644
--- a/config.c
+++ b/config.c
@@ -1281,7 +1281,7 @@ static void git_config_raw(config_fn_t fn, void *data)
 * something went really wrong and we should stop
 * immediately.
 */
-   die(_("unknown error occured while reading the configuration 
files"));
+   die(_("unknown error occurred while reading the configuration 
files"));
 }
 
 static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
diff --git a/refs.h b/refs.h
index 9230d47..56089d5 100644
--- a/refs.h
+++ b/refs.h
@@ -345,7 +345,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf 
*err);
  * msg -- a message describing the change (for the reflog).
  *
  * err -- a strbuf for receiving a description of any error that
- * might have occured.
+ * might have occurred.
  *
  * The functions make internal copies of refname and msg, so the
  * caller retains ownership of these parameters.
-- 
2.8.1

--
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 5/6] lock_ref_for_update(): make error handling more uniform

[David Turner]

On Fri, 2016-06-10 at 10:14 +0200, Michael Haggerty wrote:

>  /*
> + * Check whether the REF_HAVE_OLD and old_oid values stored in update
> + * are consistent with the result read for the reference. error is
> + * true iff there was an error reading the reference; otherwise, oid

"error" is not a thing 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

Re: [PATCH] Fix spelling of "occurred"

[Pranit Bauva]

Hey Peter,

On Fri, Jun 10, 2016 at 7:52 PM, Peter Colberg  wrote:
> Signed-off-by: Peter Colberg 
> ---
>  config.c| 2 +-
>  po/bg.po| 2 +-
>  po/ca.po| 2 +-
>  po/de.po| 2 +-
>  po/fr.po| 2 +-
>  po/git.pot  | 2 +-
>  po/ko.po| 2 +-
>  po/pt_PT.po | 2 +-
>  po/ru.po| 2 +-
>  po/sv.po| 2 +-
>  po/vi.po| 2 +-
>  po/zh_CN.po | 2 +-
>  refs.h  | 2 +-
>  13 files changed, 13 insertions(+), 13 deletions(-)
> ...[snip]

You only have to change the spelling mistake in config.c and refs.h .
Our translators take care of everything else.
Thanks for your contribution.

Regards,
Pranit Bauva
--
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: Re* [BUG-ish] diff compaction heuristic false positive

[Stefan Beller]

On Fri, Jun 10, 2016 at 11:13 AM, Junio C Hamano  wrote:
> Subject: [PATCH] diff: disable compaction heuristic for now
>
> http://lkml.kernel.org/g/20160610075043.ga13...@sigill.intra.peff.net
> reports that a change to add a new "function" with common ending
> with the existing one at the end of the file is shown like this:
>
> def foo
>   do_foo_stuff()
>
>+  common_ending()
>+end
>+
>+def bar
>+  do_bar_stuff()
>+
>   common_ending()
> end
>
> when the new heuristic is in use.  In reality, the change is to add
> the blank line before "def bar" and everything below, which is what
> the code without the new heuristic shows.
>
> Disable the heuristics by default and leave it as an experimental
> feature for now.
>
> Signed-off-by: Junio C Hamano 
> ---

Thanks,
Stefan
--
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* [BUG-ish] diff compaction heuristic false positive

[Junio C Hamano]

Jacob Keller  writes:

> I think we could use the indentation trick and it might help in this
> case. I agree, let's disable this for this cycle and experiment in the
> next one. Good catch, Peff.
>
> As others have said you will always be able to produce counter
> examples, that's the nature of heuristics. The idea is to see if we
> can come up with something simple that mostly improves the output,
> even if sometimes it might have a negative impact on the outputs. But
> I think we should avoid changing behavior unless it's mostly an
> improvement.

OK, let's do this then for the upcoming release for now.  I am
tempted to flip it back on after the release in 'next', so that
developers would be exposed to the heuristic by default, though.

-- >8 --
Subject: [PATCH] diff: disable compaction heuristic for now

http://lkml.kernel.org/g/20160610075043.ga13...@sigill.intra.peff.net
reports that a change to add a new "function" with common ending
with the existing one at the end of the file is shown like this:

def foo
  do_foo_stuff()

   +  common_ending()
   +end
   +
   +def bar
   +  do_bar_stuff()
   +
  common_ending()
end

when the new heuristic is in use.  In reality, the change is to add
the blank line before "def bar" and everything below, which is what
the code without the new heuristic shows.

Disable the heuristics by default and leave it as an experimental
feature for now.

Signed-off-by: Junio C Hamano 
---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 05ca3ce..9116d9d 100644
--- a/diff.c
+++ b/diff.c
@@ -25,7 +25,7 @@
 #endif
 
 static int diff_detect_rename_default;
-static int diff_compaction_heuristic = 1;
+static int diff_compaction_heuristic; /* experimental */
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
-- 
2.9.0-rc2-275-g493bdbb

--
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 00/94] libify apply and use lib in am

[Johannes Sixt]

Am 10.06.2016 um 13:11 schrieb Johannes Schindelin:

On Fri, 10 Jun 2016, Christian Couder wrote:

On Fri, Jun 10, 2016 at 9:01 AM, Johannes Schindelin
 wrote:

I lost track in the meantime: were those issues with unclosed file handles
and unreleased memory in the error code paths addressed systematically? My
mail about that seems to have been left unanswered, almost as if my
concerns had been hand-waved away...


Haven't I answered to your email in this thread:

http://thread.gmane.org/gmane.comp.version-control.git/292403/

?


Not really. The reply (which I had not quite connected with my mail
because they were over a week apart) says this:


I fixed this by moving the "close(fd)" call just after the
"apply_patch()" call.


This bug in v1 was discovered by the test suite and fixed in v2.



and this:


I will have another look at the 2 other places where there are
open()/close() or fopen()/fclose() calls.


but nothing about a careful, systematic investigation of all error code
paths. As a consequence, I fully expect to encounter test failures as soon
as I test your patch series again, simply because resources are still in
use when they should no longer be used. In other words, my expectations
are now lower than they have been before, my concerns are not at all
addressed.


Do you trust the test suite to some degree? It passes after the above 
bug was fixed in v2. In addition, haven't found any problems so far 
during daily use.



This is the newest iteration:

https://github.com/chriscool/git/commits/libify-apply-use-in-am65


And that cute 65 in the name is the revision.


Yeah, that number is painful. I would appreciate an unversioned branch 
name, too.


-- Hannes

--
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: [PATCHv4] Documentation: triangular workflow

[Junio C Hamano]

"Philip Oakley"  writes:

>> +Preparation
>> +~~~
>> +
>> +Cloning from **PUBLISH**, which is a fork of **UPSTREAM** or an empty
>> +repository.
>
> I agree here. To clone the upstream, to which you have no push access (by 
> definition), would leave the config badly mis-set for the basic user. It's 
> better for the user to clone their publish fork (to which they have both 
> read and write access).

I do not think I agree.

If you apriori know that you do want to hack on a project's code, then
forking at GitHub first and then cloning the copy would be OK.

But I doubt that would be a common set-up, unless you are focusing
only on school-like setting where you are told by your instructor to
"make changes to this public project, and show the result in your
fork".  In real life you cannot tell if the project is worth your
time modifying until you see it first, can you?

I suspect that the majority of local clones start from something
like "I want to build and use from the tip", "I want to use a module
that does X, and there are three candidates, so let's clone them all
to evaluate", etc.  You do not bother "forking at GitHub" but just
clone from the upstream for these clones.

After you build it and try things out, you may start making local
changes, and you may even record your changes as local commits.  You
play with your local clone of the upstream.  After doing so, you may
find that some of the projects do not fit your needs, but for some
others, you would find that it is worth your time and effort to
upstream your changes and/or keep working further on the project.

And at that point, you would create a publishing place, push into
it, and tell others "Hey I did this interesting thing!".  That
"creat a publishing place" step could be just a one click at GitHub.

Isn't that how you work with other people's projects?  Or do you
always modify every project you fetch from the outside world?, Do
you always fork first, just in case you *might* change and you
*might* have to have a place to push your changes out?

If you tell novices "You fork first and then clone your fork", and
in the ideal (to you) case they will follow that advice to the
letter and they will end up with forks of all projects they will
ever look at, in many of which they make no local commit.

What is more likely to happen is that they will first ignore you and
start from a local clone of the upstream, and then find this
document that says "triangular workflow requires you to fork first,
clone that fork and work in it".  Because they would have to fork
first and make another clone, this time a clone of the fork, in
order to follow the instruction of this document, they oblige,
ending up with two clones.  More importantly, this makes the local
clone of the upstream they made earlier and the changes they made in
that clone appear useless.  They need to be told how to transplant
the work done in the clone to the newly created clone of the fork,
in order to publish them.

If your instruction begins with "You clone from upstream as usual
(i.e. just like when you make a "read-only" clone without any
intention to make changes or push changes out), and add a publish
place if/when it becomes necessary", the problem described in the
previous paragraph goes away, no?

--
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: [BUG-ish] diff compaction heuristic false positive

[Jacob Keller]

On Fri, Jun 10, 2016 at 9:25 AM, Stefan Beller  wrote:
> On Fri, Jun 10, 2016 at 8:56 AM, Junio C Hamano  wrote:
>> Jeff King  writes:
>>
>>> On Fri, Jun 10, 2016 at 03:50:43AM -0400, Jeff King wrote:
>>>
 I found a false positive with the new compaction heuristic in v2.9:
 [...]
>>>
>>> And by the way, this is less "hey neat, I found a case" and more "wow,
>>> this is a lot worse than I thought".
>>>
>>> I diffed the old and new output for the top 10,000 commits in this
>>> particular ruby code base. There were 45 commits with changed diffs.
>>> Spot-checking them manually, a little over 1/3 of them featured this bad
>>> pattern. The others looked like strict improvements.
>>>
>>> That's a lot worse than the outcomes we saw on other code bases earlier.
>>> 1/3 bad is still a net improvement, so I dunno. Is this worth worrying
>>> about? Should we bring back the documentation for the knob to disable
>>> it? Should we consider making it tunable via gitattributes?
>>>
>>> I don't think that last one really helps; the good cases _and_ the bad
>>> ones are both in ruby code (though certainly the C code we looked at
>>> earlier was all good).
>>>
>>> It may also be possible to make it Just Work by using extra information
>>> like indentation. I haven't thought hard enough about that to say.
>>>
>>> -Peff
>>
>> I recall saying "we'd end up being better in some and worse in
>> others" at the very beginning.  How about toggling the default back
>> for the upcoming release, keeping the experimentation knob in the
>> code, and try different heuristics like the "indentation" during the
>> next cycle?
>
> Sure. I thought about for a while now and by now I agree with Junio.
> No matter what kind of heuristic we can come up with it is easy to construct
> a counter example.
>
> That said, let's try the indentation thing, though I suspect
> one of the early motivating examples (an excerpt from a  kernel config file)
> would not do well with it, as it had not an indentation scheme as programming
> languages do.
>
> Thanks,
> Stefan

I think we could use the indentation trick and it might help in this
case. I agree, let's disable this for this cycle and experiment in the
next one. Good catch, Peff.

As others have said you will always be able to produce counter
examples, that's the nature of heuristics. The idea is to see if we
can come up with something simple that mostly improves the output,
even if sometimes it might have a negative impact on the outputs. But
I think we should avoid changing behavior unless it's mostly an
improvement.

Regards,
Jake
--
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: [BUG-ish] diff compaction heuristic false positive

[Stefan Beller]

On Fri, Jun 10, 2016 at 8:56 AM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> On Fri, Jun 10, 2016 at 03:50:43AM -0400, Jeff King wrote:
>>
>>> I found a false positive with the new compaction heuristic in v2.9:
>>> [...]
>>
>> And by the way, this is less "hey neat, I found a case" and more "wow,
>> this is a lot worse than I thought".
>>
>> I diffed the old and new output for the top 10,000 commits in this
>> particular ruby code base. There were 45 commits with changed diffs.
>> Spot-checking them manually, a little over 1/3 of them featured this bad
>> pattern. The others looked like strict improvements.
>>
>> That's a lot worse than the outcomes we saw on other code bases earlier.
>> 1/3 bad is still a net improvement, so I dunno. Is this worth worrying
>> about? Should we bring back the documentation for the knob to disable
>> it? Should we consider making it tunable via gitattributes?
>>
>> I don't think that last one really helps; the good cases _and_ the bad
>> ones are both in ruby code (though certainly the C code we looked at
>> earlier was all good).
>>
>> It may also be possible to make it Just Work by using extra information
>> like indentation. I haven't thought hard enough about that to say.
>>
>> -Peff
>
> I recall saying "we'd end up being better in some and worse in
> others" at the very beginning.  How about toggling the default back
> for the upcoming release, keeping the experimentation knob in the
> code, and try different heuristics like the "indentation" during the
> next cycle?

Sure. I thought about for a while now and by now I agree with Junio.
No matter what kind of heuristic we can come up with it is easy to construct
a counter example.

That said, let's try the indentation thing, though I suspect
one of the early motivating examples (an excerpt from a  kernel config file)
would not do well with it, as it had not an indentation scheme as programming
languages do.

Thanks,
Stefan
--
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: [BUG-ish] diff compaction heuristic false positive

[Junio C Hamano]

Jeff King  writes:

> On Fri, Jun 10, 2016 at 03:50:43AM -0400, Jeff King wrote:
>
>> I found a false positive with the new compaction heuristic in v2.9:
>> [...]
>
> And by the way, this is less "hey neat, I found a case" and more "wow,
> this is a lot worse than I thought".
>
> I diffed the old and new output for the top 10,000 commits in this
> particular ruby code base. There were 45 commits with changed diffs.
> Spot-checking them manually, a little over 1/3 of them featured this bad
> pattern. The others looked like strict improvements.
>
> That's a lot worse than the outcomes we saw on other code bases earlier.
> 1/3 bad is still a net improvement, so I dunno. Is this worth worrying
> about? Should we bring back the documentation for the knob to disable
> it? Should we consider making it tunable via gitattributes?
>
> I don't think that last one really helps; the good cases _and_ the bad
> ones are both in ruby code (though certainly the C code we looked at
> earlier was all good).
>
> It may also be possible to make it Just Work by using extra information
> like indentation. I haven't thought hard enough about that to say.
>
> -Peff

I recall saying "we'd end up being better in some and worse in
others" at the very beginning.  How about toggling the default back
for the upcoming release, keeping the experimentation knob in the
code, and try different heuristics like the "indentation" during the
next cycle?
--
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 05/38] refs: create a base class "ref_store" for files_ref_store

[Junio C Hamano]

Michael Haggerty  writes:

> On 06/09/2016 06:14 PM, Junio C Hamano wrote:
>> Michael Haggerty  writes:
>> 
> +static struct ref_store *main_ref_store = NULL;
> +
> +static struct ref_store *submodule_ref_stores = NULL;

 Let's let BSS take care of these initialization.
>> [...]
>> Lack of "= ..." is a clear-enough clue that the code wants these
>> pointers initialized to NULL.
>> [...]
>
> OK. While I'm at it I'll add docstrings for these variables.

Yeah, if you explain what they are used for, it would become obvious
why their natural initial state is NULL.
--
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] Adds *~ to the .gitignore

[Junio C Hamano]

Jeff King  writes:

> On Thu, Jun 09, 2016 at 03:48:12PM -0700, Junio C Hamano wrote:
>
>> As I said, however, I could support a move to add some selected
>> small number of common file extensions, as long as we have some
>> (social) mechanism to avoid churning this file every time somebody
>> new comes and complains their favourite editor or other tools are
>> not supported.
>
> Yeah, I don't mind it either, myself, provided we avoid the churn.
>
> OTOH, wouldn't somebody who cared about this want it for all of their
> projects? I guess I just don't see the point in making this a
> git-specific thing.

My version was "it is your personal preference, why should we cater
to it while we know we cannot do the same for everybody else who has
different preference?"

Your version is a lot more positive, while leading to the same
conclusion.  "It is your personal preference.  Even if we added it
ourselves, it would not help you with other projects".

As usual, you are more brilliant than I am 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

Re: [PATCH v2 00/33] Yet more preparation for reference backends

[Junio C Hamano]

Michael Haggerty  writes:

> Junio, if you want to incorporate this revised version of the branch in
> your big rewind of next, then we can pretend that the bug was never
> there :-) Otherwise, tell me in what form you would like the fix and I
> will be happy to provide it.

We actually can do both ;-).  That is, when we rewind 'next' after
the upcoming release, we'd queue a series without a bug, but you can
show a preview to the list in a form of an incremental patch so that
others have a chance to see how the end result would look like.

Thanks.
--
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] Fix spelling of "occurred"

[Peter Colberg]

Signed-off-by: Peter Colberg 
---
 config.c| 2 +-
 po/bg.po| 2 +-
 po/ca.po| 2 +-
 po/de.po| 2 +-
 po/fr.po| 2 +-
 po/git.pot  | 2 +-
 po/ko.po| 2 +-
 po/pt_PT.po | 2 +-
 po/ru.po| 2 +-
 po/sv.po| 2 +-
 po/vi.po| 2 +-
 po/zh_CN.po | 2 +-
 refs.h  | 2 +-
 13 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/config.c b/config.c
index f51c56b..d7ce34b 100644
--- a/config.c
+++ b/config.c
@@ -1281,7 +1281,7 @@ static void git_config_raw(config_fn_t fn, void *data)
 * something went really wrong and we should stop
 * immediately.
 */
-   die(_("unknown error occured while reading the configuration 
files"));
+   die(_("unknown error occurred while reading the configuration 
files"));
 }
 
 static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
diff --git a/po/bg.po b/po/bg.po
index ac6f103..462eb9a 100644
--- a/po/bg.po
+++ b/po/bg.po
@@ -498,7 +498,7 @@ msgid "unable to parse command-line config"
 msgstr "неправилни настройки от командния ред"
 
 #: config.c:1277
-msgid "unknown error occured while reading the configuration files"
+msgid "unknown error occurred while reading the configuration files"
 msgstr "неочаквана грешка при изчитането на конфигурационните файлове"
 
 #: config.c:1601
diff --git a/po/ca.po b/po/ca.po
index 46000d7..b3dbba7 100644
--- a/po/ca.po
+++ b/po/ca.po
@@ -419,7 +419,7 @@ msgid "unable to parse command-line config"
 msgstr "no s'ha pogut analitzar la configuració de la línia d'ordres"
 
 #: config.c:1281
-msgid "unknown error occured while reading the configuration files"
+msgid "unknown error occurred while reading the configuration files"
 msgstr "ha ocorregut un error desconegut en llegir els fitxers de configuració"
 
 #: config.c:1629
diff --git a/po/de.po b/po/de.po
index 0eadf34..5bf91a0 100644
--- a/po/de.po
+++ b/po/de.po
@@ -417,7 +417,7 @@ msgstr ""
 "Konnte die über die Befehlszeile angegebene Konfiguration nicht parsen."
 
 #: config.c:1281
-msgid "unknown error occured while reading the configuration files"
+msgid "unknown error occurred while reading the configuration files"
 msgstr ""
 "Es trat ein unbekannter Fehler beim Lesen der Konfigurationsdateien auf."
 
diff --git a/po/fr.po b/po/fr.po
index 55ca387..81465a9 100644
--- a/po/fr.po
+++ b/po/fr.po
@@ -483,7 +483,7 @@ msgid "unable to parse command-line config"
 msgstr "lecture de la configuration de ligne de commande impossible"
 
 #: config.c:1281
-msgid "unknown error occured while reading the configuration files"
+msgid "unknown error occurred while reading the configuration files"
 msgstr "erreur inconnue pendant la lecture des fichiers de configuration"
 
 #: config.c:1629
diff --git a/po/git.pot b/po/git.pot
index 72ef798..7a3e9ba 100644
--- a/po/git.pot
+++ b/po/git.pot
@@ -390,7 +390,7 @@ msgid "unable to parse command-line config"
 msgstr ""
 
 #: config.c:1281
-msgid "unknown error occured while reading the configuration files"
+msgid "unknown error occurred while reading the configuration files"
 msgstr ""
 
 #: config.c:1629
diff --git a/po/ko.po b/po/ko.po
index 3ff3b9b..f1017b8 100644
--- a/po/ko.po
+++ b/po/ko.po
@@ -460,7 +460,7 @@ msgid "unable to parse command-line config"
 msgstr "명령행 설정을 파싱할 수 없습니다"
 
 #: config.c:1281
-msgid "unknown error occured while reading the configuration files"
+msgid "unknown error occurred while reading the configuration files"
 msgstr "설정 파일을 읽는 중 알 수 없는 오류가 생겼습니다"
 
 #: config.c:1629
diff --git a/po/pt_PT.po b/po/pt_PT.po
index 321b553..896eb2a 100644
--- a/po/pt_PT.po
+++ b/po/pt_PT.po
@@ -410,7 +410,7 @@ msgid "unable to parse command-line config"
 msgstr "não é possível analisar configuração de linha de comandos"
 
 #: config.c:1281
-msgid "unknown error occured while reading the configuration files"
+msgid "unknown error occurred while reading the configuration files"
 msgstr ""
 "ocorreu um erro desconhecido durante a leitura dos ficheiros de configuração"
 
diff --git a/po/ru.po b/po/ru.po
index c0a838b..974b9ee 100644
--- a/po/ru.po
+++ b/po/ru.po
@@ -398,7 +398,7 @@ msgid "unable to parse command-line config"
 msgstr "не удалось разобрать конфигурацию из командной строки"
 
 #: config.c:1281
-msgid "unknown error occured while reading the configuration files"
+msgid "unknown error occurred while reading the configuration files"
 msgstr "произошла неизвестная ошибка при чтении файлов конфигурации"
 
 #: config.c:1629
diff --git a/po/sv.po b/po/sv.po
index 32bcaba..3d89dcc 100644
--- a/po/sv.po
+++ b/po/sv.po
@@ -410,7 +410,7 @@ msgid "unable to parse command-line config"
 msgstr "kan inte tolka kommandoradskonfiguration"
 
 #: config.c:1281
-msgid "unknown error occured while reading the configuration files"
+msgid "unknown error occurred while reading the configuration files"
 msgstr "okänt fel uppstod vid läsning av konfigurationsfilerna"
 
 #: config.c:1629
diff --git a/po/vi.po 

Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C

[Pranit Bauva]

Hey Eric,

On Fri, Jun 10, 2016 at 3:03 AM, Eric Sunshine  wrote:
> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva  wrote:
>> Reimplement `is_expected_rev` shell function in C. This will further be
>> called from `check_expected_revs` function. This is a quite small
>> function thus subcommand facility is redundant.
>
> This patch should be squashed into patch 2/2, as it is otherwise
> pointless without that patch, and merely adds dead code.

Sure I will squash and will explain it in the commit message.

>> Signed-off-by: Pranit Bauva 
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -160,6 +160,20 @@ int bisect_reset(const char *commit)
>> +static int is_expected_rev(const char *expected_hex)
>> +{
>> +   struct strbuf actual_hex = STRBUF_INIT;
>> +
>> +   if (!file_exists(git_path_bisect_expected_rev()))
>> +   return 0;
>
> Invoking file_exists() seems unnecessarily redundant when you can
> discern effectively the same by checking the return value of
> strbuf_read_file() below. I'd drop the file_exists() check altogether.

I wanted to imitate the code. But I guess it would actually be better
if I drop this file_exists().

>> +   if (!strbuf_read_file(_hex, git_path_bisect_expected_rev(), 
>> 0))
>> +   return 0;
>
> What exactly is this trying to do? Considering that strbuf_read_file()
> returns -1 upon error, otherwise the number of bytes read, if I'm
> reading this correctly, is_expected_rev() returns false if
> strbuf_read_file() encounters an error (which is fine) but also when
> it successfully reads the file and its content length is non-zero
> (which is very odd).
>
>> +   strbuf_trim(_hex);
>> +   return !strcmp(actual_hex.buf, expected_hex);
>
> Thus, it only ever gets to this point if the file exists but is empty,
> which is very unlikely to match 'expected_hex'. I could understand it
> if you checked the result of strbuf_read_file() with <0 or even <=0,
> but the current code doesn't make sense to me.
>
> Am I misunderstanding?


Definitely not. Thanks for pointing it out. :) It went off my head
that strbuf_read_file returns the bytes it reads. Also the code
comment regarding strbuf_read_file does not mention it which probably
misguided me. I should also send a fixing patch so that someone else
does not fall into this like I did.

I will also release the 'actual_hex'.

Thanks for your comments!

Regards,
Pranit Bauva
--
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 v4 2/3] completion: add __git_get_option_value helper

[SZEDER Gábor]

Hallo Thomas,

I saw v5 hit my mailbox while writing this.  I glanced it over and it
seems my comments here apply to that version as well.

Quoting Thomas Braun :


This function allows to search the commmand line and config
files for an option, long and short, with mandatory value.

The function would return e.g. for the command line
"git status -uno --untracked-files=all" the result
"all" regardless of the config option.


Wow, regarding my earlier remark about bonus points: I didn't realize
that there were so many bonus point to give away :)


Signed-off-by: Thomas Braun 
---
contrib/completion/git-completion.bash | 44  
++

1 file changed, 44 insertions(+)

diff --git a/contrib/completion/git-completion.bash  
b/contrib/completion/git-completion.bash

index addea89..4bd17aa 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -803,6 +803,50 @@ __git_find_on_cmdline ()
done
}

+# Echo the value of an option set on the command line or config
+#
+# $1: short option name
+# $2: long option name including =


I'm not sure about requiring the '=', the function could just append
it as necessary.  More on this below.


+# $3: list of possible values
+# $4: config string (optional)


I don't understand why the list of possible values is necessary.

This function will be called when the caller wants to take different
actions based on different values, so the caller will process the
function's output with a case statement or an if-else chain, both of
which would be perfectly capable to ignore whatever invalid value the
user might have specified.  Therefore, I think this function doesn't
need the list of possible values, it should just return whatever value
it found after the option.


+# example:
+# result="$(__git_get_option_value "-d" "--do-something="\
+# "yes no" "core.doSomething")"
+#
+# result is then either empty (no option set) or "yes" or "no"
+#
+# __git_get_option_value requires 3 arguments
+__git_get_option_value ()
+{
+   local c short_opt long_opt val
+   local result= values config_key word
+
+   short_opt="$1"
+   long_opt="$2"
+   values="$3"
+   config_key="$4"


These can be assigned when the variables are declared, saving a couple
of lines.


+   ((c = $cword - 1))
+   while [ $c -ge 0 ]; do


Searching from the end of the command line, so even if someone were to
do a 'git status -uall -unormal -uno ', this would still do the
right thing.  Good!

However ;)
Just for fun imagine following:

  $ >-uno
  $ git status -- -uno 

'git status' treats that '-uno' after the doubledash as a filename,
but this function interprets it as an option, and on the subsequent
TAB the completion script won't list untracked files.

I'm tempted to say that this is such a pathological corner case that
it doesn't worth worrying about.


+   word="${words[c]}"
+   for val in $values; do


Without the possible values argument this inner loop could go away.


+   if [ "$short_opt$val" = "$word" ]
+   || [ "$long_opt$val"  = "$word" ]; then
+   result="$val"
+   break 2


You could just 'echo "$val"' or rather ${word#$short_opt} and return
here ...


+   fi
+   done
+   ((c--))
+   done
+
+   if [ -n "$config_key" ] && [ -z "$result" ]; then


... and that would make the second condition unnecessary here ...


+   result="$(git --git-dir="$(__gitdir)" config "$config_key")"


... and this could just be a simple 'git config' execution, without
command substitution ...


+   fi
+
+   echo "$result"


... and this echo could go away as well.


+}
+
__git_has_doubledash ()
{
local c=1
--
2.8.3.windows.1



However, I'm not sure we need or want this helper function _at the
moment_.  Yes, in general helper functions are good, and in this case
it makes _git_status() easier to follow, but it has some drawbacks,
too:

  - It has a single callsite: the upcoming _git_status().  No other
existing case springs to mind where it could be used, i.e. where
different values of an option would require different actions from
the completion script.  Maybe we'll have one in the future, maybe
not.

  - This function works only with the "stuck" form of options, i.e.
'--opt=val' or '-oval', which is mostly sufficient in this case,
because 'git status' understands only this form.  However, it
doesn't work with "unstuck" options, i.e. '--opt val' or '-o val'.
In many cases git supports only this "unstuck" form, and there are
many cases where it supports both for a given option.  We can't know
which form a future callsite might need, but requiring the '=' as
part of the long option seems to paint us into a 

Re: [PATCH v2 00/33] Yet more preparation for reference backends

[Michael Haggerty]

On 05/06/2016 06:13 PM, Michael Haggerty wrote:
> [...]
> This patch series is also available from my GitHub repo [2] as branch
> "split-under-lock".
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/292772
> [2] https://github.com/mhagger/git

I was reading this area of the code again, and I found a problem in this
patch series. Now that ref_updates can be added to ref_transactions
while they are being processed, it is not correct to store a pointer to
transaction->updates in ref_transaction_commit() here [1], because the
array might be moved by realloc() if it grows during the function. The
problem wasn't detected during testing because an added commit would
have to cross an ALLOC_GROW boundary to trigger the bug.

The fix is obvious but it is textually quite a few lines. For good
measure, the same fix should be made in initial_ref_transaction_commit()
here [2].

The most logical place to fix this is by expanding commit
"ref_transaction_commit(): remove local variable n" [3], so I've done
that and force pushed the result to my GitHub account [4] as branch
"split-under-lock".

Junio, if you want to incorporate this revised version of the branch in
your big rewind of next, then we can pretend that the bug was never
there :-) Otherwise, tell me in what form you would like the fix and I
will be happy to provide it.

Sorry for finding this problem so late in the process.

Michael

[1]
https://github.com/mhagger/git/blob/088c8f756c86581ff25371983ef409044b348bb9/refs/files-backend.c#L3559
[2]
https://github.com/mhagger/git/blob/088c8f756c86581ff25371983ef409044b348bb9/refs/files-backend.c#L3725
[3] http://article.gmane.org/gmane.comp.version-control.git/293801
[4] https://github.com/mhagger/git

--
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 27/27] fetch, upload-pack: --deepen=N extends shallow boundary by N commits

[Nguyễn Thái Ngọc Duy]

In git-fetch, --depth argument is always relative with the latest
remote refs. This makes it a bit difficult to cover this use case,
where the user wants to make the shallow history, say 3 levels
deeper. It would work if remote refs have not moved yet, but nobody
can guarantee that, especially when that use case is performed a
couple months after the last clone or "git fetch --depth". Also,
modifying shallow boundary using --depth does not work well with
clones created by --since or --not.

This patch fixes that. A new argument --deepen= will add  more (*)
parent commits to the current history regardless of where remote refs
are.

Have/Want negotiation is still respected. So if remote refs move, the
server will send two chunks: one between "have" and "want" and another
to extend shallow history. In theory, the client could send no "want"s
in order to get the second chunk only. But the protocol does not allow
that. Either you send no want lines, which means ls-remote; or you
have to send at least one want line that carries deep-relative to the
server..

The main work was done by Dongcan Jiang. I fixed it up here and there.
And of course all the bugs belong to me.

(*) We could even support --deepen= where  is negative. In that
case we can cut some history from the shallow clone. This operation
(and --depth=) does not require interaction with remote
side (and more complicated to implement as a result).

Helped-by: Duy Nguyen 
Helped-by: Eric Sunshine 
Helped-by: Junio C Hamano 
Signed-off-by: Dongcan Jiang 
Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 Documentation/fetch-options.txt   |  5 +
 Documentation/git-fetch-pack.txt  |  5 +
 Documentation/gitremote-helpers.txt   |  4 
 Documentation/technical/protocol-capabilities.txt |  7 ++
 builtin/fetch-pack.c  |  4 
 builtin/fetch.c   | 14 +++-
 fetch-pack.c  |  3 +++
 fetch-pack.h  |  1 +
 remote-curl.c | 14 +++-
 t/t5500-fetch-pack.sh | 23 
 t/t5539-fetch-http-shallow.sh | 26 +++
 transport-helper.c|  1 +
 transport.c   |  4 
 transport.h   |  4 
 upload-pack.c | 21 +-
 15 files changed, 129 insertions(+), 7 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 7aa1285..3b91f15 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -14,6 +14,11 @@
linkgit:git-clone[1]), deepen or shorten the history to the specified
number of commits. Tags for the deepened commits are not fetched.
 
+--deepen=::
+   Similar to --depth, except it specifies the number of commits
+   from the current shallow boundary instead of from the tip of
+   each remote branch history.
+
 --shallow-since=::
Deepen or shorten the history of a shallow repository to
include all reachable commits after .
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 4d15b04..c20958f 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -96,6 +96,11 @@ be in a separate packet, and the list must end with a flush 
packet.
exclude commits reachable from a specified remote branch or tag.
This option can be specified multiple times.
 
+--deepen-relative::
+   Argument --depth specifies the number of commits from the
+   current shallow boundary instead of from the tip of each
+   remote branch history.
+
 --no-progress::
Do not show the progress.
 
diff --git a/Documentation/gitremote-helpers.txt 
b/Documentation/gitremote-helpers.txt
index 75bb638..6fca268 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -422,6 +422,10 @@ set by Git if the remote helper has the 'option' 
capability.
Deepens the history of a shallow repository excluding ref.
Multiple options add up.
 
+'option deepen-relative {'true'|'false'}::
+   Deepens the history of a shallow repository relative to
+   current boundary. Only valid when used with "option depth".
+
 'option followtags' {'true'|'false'}::
If enabled the helper should automatically fetch annotated
tag objects if the object the tag points at was transferred
diff --git a/Documentation/technical/protocol-capabilities.txt 
b/Documentation/technical/protocol-capabilities.txt
index 0e6b57d..4fd6dcc 100644
--- 

[PATCH 23/27] clone: define shallow clone boundary with --shallow-exclude

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-clone.txt |  5 +
 builtin/clone.c | 10 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index a410409..5049663 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -196,6 +196,11 @@ objects from the source repository into a pack in the 
cloned repository.
 --shallow-since=::
Create a shallow clone with a history after the specified time.
 
+--shallow-exclude=::
+   Create a shallow clone with a history, excluding commits
+   reachable from a specified remote branch or tag.  This option
+   can be specified multiple times.
+
 --[no-]single-branch::
Clone only the history leading to the tip of a single branch,
either specified by the `--branch` option or the primary
diff --git a/builtin/clone.c b/builtin/clone.c
index dc2ef4f..3849231 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -44,6 +44,7 @@ static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
 static char *option_branch = NULL;
+static struct string_list option_not = STRING_LIST_INIT_NODUP;
 static const char *real_git_dir;
 static char *option_upload_pack = "git-upload-pack";
 static int option_verbosity;
@@ -89,6 +90,8 @@ static struct option builtin_clone_options[] = {
N_("create a shallow clone of that depth")),
OPT_STRING(0, "shallow-since", _since, N_("time"),
N_("create a shallow clone since a specific time")),
+   OPT_STRING_LIST(0, "shallow-exclude", _not, N_("revision"),
+   N_("deepen history of shallow clone by excluding rev")),
OPT_BOOL(0, "single-branch", _single_branch,
N_("clone only one branch, HEAD or --branch")),
OPT_STRING(0, "separate-git-dir", _git_dir, N_("gitdir"),
@@ -852,7 +855,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
usage_msg_opt(_("You must specify a repository to clone."),
builtin_clone_usage, builtin_clone_options);
 
-   if (option_depth || option_since)
+   if (option_depth || option_since || option_not.nr)
deepen = 1;
if (option_single_branch == -1)
option_single_branch = deepen ? 1 : 0;
@@ -983,6 +986,8 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
warning(_("--depth is ignored in local clones; use 
file:// instead."));
if (option_since)
warning(_("--shallow-since is ignored in local clones; 
use file:// instead."));
+   if (option_not.nr)
+   warning(_("--shallow-exclude is ignored in local 
clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
if (option_local > 0)
warning(_("source repository is shallow, 
ignoring --local"));
@@ -1004,6 +1009,9 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (option_since)
transport_set_option(transport, TRANS_OPT_DEEPEN_SINCE,
 option_since);
+   if (option_not.nr)
+   transport_set_option(transport, TRANS_OPT_DEEPEN_NOT,
+(const char *)_not);
if (option_single_branch)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 
-- 
2.8.2.524.g6ff3d78

--
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 26/27] upload-pack: add get_reachable_list()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 object.h  |  2 +-
 upload-pack.c | 52 +---
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/object.h b/object.h
index f8b6442..614a006 100644
--- a/object.h
+++ b/object.h
@@ -31,7 +31,7 @@ struct object_array {
  * revision.h:  0-1026
  * fetch-pack.c:0---4
  * walker.c:0-2
- * upload-pack.c:   1119
+ * upload-pack.c:   4   1119
  * builtin/blame.c:   12-13
  * bisect.c:   16
  * bundle.c:   16
diff --git a/upload-pack.c b/upload-pack.c
index 5942f99..1d4f914 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -453,7 +453,8 @@ static int is_our_ref(struct object *o)
 }
 
 static int do_reachable_revlist(struct child_process *cmd,
-   struct object_array *src)
+   struct object_array *src,
+   struct object_array *reachable)
 {
static const char *argv[] = {
"rev-list", "--stdin", NULL,
@@ -484,6 +485,8 @@ static int do_reachable_revlist(struct child_process *cmd,
o = get_indexed_object(--i);
if (!o)
continue;
+   if (reachable && o->type == OBJ_COMMIT)
+   o->flags &= ~TMP_MARK;
if (!is_our_ref(o))
continue;
memcpy(namebuf + 1, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
@@ -495,8 +498,13 @@ static int do_reachable_revlist(struct child_process *cmd,
namebuf[40] = '\n';
for (i = 0; i < src->nr; i++) {
o = src->objects[i].item;
-   if (is_our_ref(o))
+   if (is_our_ref(o)) {
+   if (reachable)
+   add_object_array(o, NULL, reachable);
continue;
+   }
+   if (reachable && o->type == OBJ_COMMIT)
+   o->flags |= TMP_MARK;
memcpy(namebuf, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
if (write_in_full(cmd->in, namebuf, 41) < 0) {
sigchain_pop(SIGPIPE);
@@ -517,13 +525,51 @@ error:
return -1;
 }
 
+static int get_reachable_list(struct object_array *src,
+ struct object_array *reachable)
+{
+   struct child_process cmd = CHILD_PROCESS_INIT;
+   int i, ret = do_reachable_revlist(, src, reachable);
+   struct object *o;
+   char namebuf[42]; /* ^ + SHA-1 + LF */
+
+   if (ret < 0)
+   return -1;
+
+   while ((i = read_in_full(cmd.out, namebuf, 41)) == 41) {
+   struct object_id sha1;
+
+   if (namebuf[40] != '\n' || get_oid_hex(namebuf, ))
+   break;
+
+   o = lookup_object(sha1.hash);
+   if (o && o->type == OBJ_COMMIT) {
+   o->flags &= ~TMP_MARK;
+   }
+   }
+   for (i = get_max_object_index(); 0 < i; i--) {
+   o = get_indexed_object(i - 1);
+   if (o && o->type == OBJ_COMMIT &&
+   (o->flags & TMP_MARK)) {
+   add_object_array(o, NULL, reachable);
+   o->flags &= ~TMP_MARK;
+   }
+   }
+   close(cmd.out);
+
+   if (finish_command())
+   return -1;
+
+   return 0;
+}
+
 static int check_unreachable(struct object_array *src)
 {
struct child_process cmd = CHILD_PROCESS_INIT;
char buf[1];
int i;
 
-   if (do_reachable_revlist(, src) < 0)
+   if (do_reachable_revlist(, src, NULL) < 0)
return 0;
 
/*
-- 
2.8.2.524.g6ff3d78

--
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 25/27] upload-pack: split check_unreachable() in two, prep for get_reachable_list()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 upload-pack.c | 44 +++-
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 3f40fcb..5942f99 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -452,23 +452,23 @@ static int is_our_ref(struct object *o)
return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
 
-static int check_unreachable(struct object_array *src)
+static int do_reachable_revlist(struct child_process *cmd,
+   struct object_array *src)
 {
static const char *argv[] = {
"rev-list", "--stdin", NULL,
};
-   static struct child_process cmd = CHILD_PROCESS_INIT;
struct object *o;
char namebuf[42]; /* ^ + SHA-1 + LF */
int i;
 
-   cmd.argv = argv;
-   cmd.git_cmd = 1;
-   cmd.no_stderr = 1;
-   cmd.in = -1;
-   cmd.out = -1;
+   cmd->argv = argv;
+   cmd->git_cmd = 1;
+   cmd->no_stderr = 1;
+   cmd->in = -1;
+   cmd->out = -1;
 
-   if (start_command())
+   if (start_command(cmd))
goto error;
 
/*
@@ -487,7 +487,7 @@ static int check_unreachable(struct object_array *src)
if (!is_our_ref(o))
continue;
memcpy(namebuf + 1, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
-   if (write_in_full(cmd.in, namebuf, 42) < 0) {
+   if (write_in_full(cmd->in, namebuf, 42) < 0) {
sigchain_pop(SIGPIPE);
goto error;
}
@@ -498,21 +498,39 @@ static int check_unreachable(struct object_array *src)
if (is_our_ref(o))
continue;
memcpy(namebuf, oid_to_hex(>oid), GIT_SHA1_HEXSZ);
-   if (write_in_full(cmd.in, namebuf, 41) < 0) {
+   if (write_in_full(cmd->in, namebuf, 41) < 0) {
sigchain_pop(SIGPIPE);
goto error;
}
}
-   close(cmd.in);
-   cmd.in = -1;
+   close(cmd->in);
+   cmd->in = -1;
 
sigchain_pop(SIGPIPE);
+   return 0;
+
+error:
+   if (cmd->in >= 0)
+   close(cmd->in);
+   if (cmd->out >= 0)
+   close(cmd->out);
+   return -1;
+}
+
+static int check_unreachable(struct object_array *src)
+{
+   struct child_process cmd = CHILD_PROCESS_INIT;
+   char buf[1];
+   int i;
+
+   if (do_reachable_revlist(, src) < 0)
+   return 0;
 
/*
 * The commits out of the rev-list are not ancestors of
 * our ref.
 */
-   i = read_in_full(cmd.out, namebuf, 1);
+   i = read_in_full(cmd.out, buf, 1);
if (i)
goto error;
close(cmd.out);
-- 
2.8.2.524.g6ff3d78

--
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 24/27] t5500, t5539: tests for shallow depth excluding a ref

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t5500-fetch-pack.sh | 21 +
 t/t5539-fetch-http-shallow.sh | 22 ++
 2 files changed, 43 insertions(+)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 26f050d..145b370 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -661,4 +661,25 @@ test_expect_success 'fetch shallow since ...' '
test_cmp expected actual
 '
 
+test_expect_success 'shallow clone exclude tag two' '
+   test_create_repo shallow-exclude &&
+   (
+   cd shallow-exclude &&
+   test_commit one &&
+   test_commit two &&
+   test_commit three &&
+   git clone --shallow-exclude two "file://$(pwd)/." ../shallow12 &&
+   git -C ../shallow12 log --pretty=tformat:%s HEAD >actual &&
+   echo three >expected &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'fetch exclude tag one' '
+   git -C shallow12 fetch --shallow-exclude one origin &&
+   git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
+   test_write_lines three two >expected &&
+   test_cmp expected actual
+'
+
 test_done
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 704753c..8e38c1b 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -98,5 +98,27 @@ test_expect_success 'fetch shallow since ...' '
test_cmp expected actual
 '
 
+test_expect_success 'shallow clone exclude tag two' '
+   test_create_repo shallow-exclude &&
+   (
+   cd shallow-exclude &&
+   test_commit one &&
+   test_commit two &&
+   test_commit three &&
+   mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-exclude.git" &&
+   git clone --shallow-exclude two $HTTPD_URL/smart/shallow-exclude.git 
../shallow12 &&
+   git -C ../shallow12 log --pretty=tformat:%s HEAD >actual &&
+   echo three >expected &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'fetch exclude tag one' '
+   git -C shallow12 fetch --shallow-exclude one origin &&
+   git -C shallow12 log --pretty=tformat:%s origin/master >actual &&
+   test_write_lines three two >expected &&
+   test_cmp expected actual
+'
+
 stop_httpd
 test_done
-- 
2.8.2.524.g6ff3d78

--
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 19/27] t5500, t5539: tests for shallow depth since a specific date

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t5500-fetch-pack.sh | 24 
 t/t5539-fetch-http-shallow.sh | 25 +
 2 files changed, 49 insertions(+)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index e5f83bf..26f050d 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -637,4 +637,28 @@ test_expect_success MINGW 'fetch-pack --diag-url c:repo' '
check_prot_path c:repo file c:repo
 '
 
+test_expect_success 'clone shallow since ...' '
+   test_create_repo shallow-since &&
+   (
+   cd shallow-since &&
+   GIT_COMMITTER_DATE="1 +0700" git commit --allow-empty -m one &&
+   GIT_COMMITTER_DATE="2 +0700" git commit --allow-empty -m two &&
+   GIT_COMMITTER_DATE="3 +0700" git commit --allow-empty -m three 
&&
+   git clone --shallow-since "3 +0700" "file://$(pwd)/." 
../shallow11 &&
+   git -C ../shallow11 log --pretty=tformat:%s HEAD >actual &&
+   echo three >expected &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'fetch shallow since ...' '
+   git -C shallow11 fetch --shallow-since "2 +0700" origin &&
+   git -C shallow11 log --pretty=tformat:%s origin/master >actual &&
+   cat >expected <<-\EOF &&
+   three
+   two
+   EOF
+   test_cmp expected actual
+'
+
 test_done
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 37a4335..704753c 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -73,5 +73,30 @@ test_expect_success 'no shallow lines after receiving ACK 
ready' '
)
 '
 
+test_expect_success 'clone shallow since ...' '
+   test_create_repo shallow-since &&
+   (
+   cd shallow-since &&
+   GIT_COMMITTER_DATE="1 +0700" git commit --allow-empty -m one &&
+   GIT_COMMITTER_DATE="2 +0700" git commit --allow-empty -m two &&
+   GIT_COMMITTER_DATE="3 +0700" git commit --allow-empty -m three 
&&
+   mv .git "$HTTPD_DOCUMENT_ROOT_PATH/shallow-since.git" &&
+   git clone --shallow-since "3 +0700" 
$HTTPD_URL/smart/shallow-since.git ../shallow11 &&
+   git -C ../shallow11 log --pretty=tformat:%s HEAD >actual &&
+   echo three >expected &&
+   test_cmp expected actual
+   )
+'
+
+test_expect_success 'fetch shallow since ...' '
+   git -C shallow11 fetch --shallow-since "2 +0700" origin &&
+   git -C shallow11 log --pretty=tformat:%s origin/master >actual &&
+   cat >expected <<-\EOF &&
+   three
+   two
+   EOF
+   test_cmp expected actual
+'
+
 stop_httpd
 test_done
-- 
2.8.2.524.g6ff3d78

--
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 18/27] clone: define shallow clone boundary based on time with --shallow-since

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 Documentation/git-clone.txt |  3 +++
 builtin/clone.c | 16 +---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index b7c467a..a410409 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -193,6 +193,9 @@ objects from the source repository into a pack in the 
cloned repository.
`--no-single-branch` is given to fetch the histories near the
tips of all branches.
 
+--shallow-since=::
+   Create a shallow clone with a history after the specified time.
+
 --[no-]single-branch::
Clone only the history leading to the tip of a single branch,
either specified by the `--branch` option or the primary
diff --git a/builtin/clone.c b/builtin/clone.c
index bcba080..dc2ef4f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -40,7 +40,8 @@ static const char * const builtin_clone_usage[] = {
 
 static int option_no_checkout, option_bare, option_mirror, 
option_single_branch = -1;
 static int option_local = -1, option_no_hardlinks, option_shared, 
option_recursive;
-static char *option_template, *option_depth;
+static int deepen;
+static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
 static char *option_branch = NULL;
 static const char *real_git_dir;
@@ -86,6 +87,8 @@ static struct option builtin_clone_options[] = {
   N_("path to git-upload-pack on the remote")),
OPT_STRING(0, "depth", _depth, N_("depth"),
N_("create a shallow clone of that depth")),
+   OPT_STRING(0, "shallow-since", _since, N_("time"),
+   N_("create a shallow clone since a specific time")),
OPT_BOOL(0, "single-branch", _single_branch,
N_("clone only one branch, HEAD or --branch")),
OPT_STRING(0, "separate-git-dir", _git_dir, N_("gitdir"),
@@ -849,8 +852,10 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
usage_msg_opt(_("You must specify a repository to clone."),
builtin_clone_usage, builtin_clone_options);
 
+   if (option_depth || option_since)
+   deepen = 1;
if (option_single_branch == -1)
-   option_single_branch = option_depth ? 1 : 0;
+   option_single_branch = deepen ? 1 : 0;
 
if (option_mirror)
option_bare = 1;
@@ -976,6 +981,8 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (is_local) {
if (option_depth)
warning(_("--depth is ignored in local clones; use 
file:// instead."));
+   if (option_since)
+   warning(_("--shallow-since is ignored in local clones; 
use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
if (option_local > 0)
warning(_("source repository is shallow, 
ignoring --local"));
@@ -994,6 +1001,9 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
 option_depth);
+   if (option_since)
+   transport_set_option(transport, TRANS_OPT_DEEPEN_SINCE,
+option_since);
if (option_single_branch)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
 
@@ -1001,7 +1011,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
transport_set_option(transport, TRANS_OPT_UPLOADPACK,
 option_upload_pack);
 
-   if (transport->smart_options && !option_depth)
+   if (transport->smart_options && !deepen)
transport->smart_options->check_self_contained_and_connected = 
1;
 
refs = transport_get_remote_refs(transport);
-- 
2.8.2.524.g6ff3d78

--
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 20/27] refs: add expand_ref()

[Nguyễn Thái Ngọc Duy]

This is basically dwim_ref() without @{} support. To be used on the
server side where we want to expand abbreviated to full ref names and
nothing else. The first user is "git clone/fetch --shallow-exclude".

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 refs.c | 8 +++-
 refs.h | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index e2d34b2..842e4d8 100644
--- a/refs.c
+++ b/refs.c
@@ -392,6 +392,13 @@ static char *substitute_branch_name(const char **string, 
int *len)
 int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 {
char *last_branch = substitute_branch_name(, );
+   int   refs_found  = expand_ref(str, len, sha1, ref);
+   free(last_branch);
+   return refs_found;
+}
+
+int expand_ref(const char *str, int len, unsigned char *sha1, char **ref)
+{
const char **p, *r;
int refs_found = 0;
 
@@ -417,7 +424,6 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, 
char **ref)
warning("ignoring broken ref %s.", fullref);
}
}
-   free(last_branch);
return refs_found;
 }
 
diff --git a/refs.h b/refs.h
index 3c3da29..31a2fa6 100644
--- a/refs.h
+++ b/refs.h
@@ -90,6 +90,7 @@ extern int resolve_gitlink_ref(const char *path, const char 
*refname, unsigned c
  */
 extern int refname_match(const char *abbrev_name, const char *full_name);
 
+extern int expand_ref(const char *str, int len, unsigned char *sha1, char 
**ref);
 extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
 extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
 
-- 
2.8.2.524.g6ff3d78

--
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 22/27] fetch: define shallow boundary with --shallow-exclude

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/fetch-options.txt |  5 +
 Documentation/git-fetch-pack.txt|  5 +
 Documentation/gitremote-helpers.txt |  4 
 builtin/fetch-pack.c|  7 +++
 builtin/fetch.c | 13 ++---
 fetch-pack.c| 15 ++-
 fetch-pack.h|  1 +
 remote-curl.c   |  9 +
 transport-helper.c  | 24 
 transport.c |  4 
 transport.h |  6 ++
 11 files changed, 89 insertions(+), 4 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 8738d3d..7aa1285 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -18,6 +18,11 @@
Deepen or shorten the history of a shallow repository to
include all reachable commits after .
 
+--shallow-exclude=::
+   Deepen or shorten the history of a shallow repository to
+   exclude commits reachable from a specified remote branch or tag.
+   This option can be specified multiple times.
+
 --unshallow::
If the source repository is complete, convert a shallow
repository to a complete one, removing all the limitations
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 99e6257..4d15b04 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -91,6 +91,11 @@ be in a separate packet, and the list must end with a flush 
packet.
Deepen or shorten the history of a shallow'repository to
include all reachable commits after .
 
+--shallow-exclude=::
+   Deepen or shorten the history of a shallow repository to
+   exclude commits reachable from a specified remote branch or tag.
+   This option can be specified multiple times.
+
 --no-progress::
Do not show the progress.
 
diff --git a/Documentation/gitremote-helpers.txt 
b/Documentation/gitremote-helpers.txt
index 9971d9a..75bb638 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -418,6 +418,10 @@ set by Git if the remote helper has the 'option' 
capability.
 'option deepen-since ::
Deepens the history of a shallow repository based on time.
 
+'option deepen-not ::
+   Deepens the history of a shallow repository excluding ref.
+   Multiple options add up.
+
 'option followtags' {'true'|'false'}::
If enabled the helper should automatically fetch annotated
tag objects if the object the tag points at was transferred
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 0402e27..07570be 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -50,6 +50,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
struct child_process *conn;
struct fetch_pack_args args;
struct sha1_array shallow = SHA1_ARRAY_INIT;
+   struct string_list deepen_not = STRING_LIST_INIT_DUP;
 
packet_trace_identity("fetch-pack");
 
@@ -108,6 +109,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
args.deepen_since = xstrdup(arg);
continue;
}
+   if (skip_prefix(arg, "--shallow-exclude=", )) {
+   string_list_append(_not, arg);
+   continue;
+   }
if (!strcmp("--no-progress", arg)) {
args.no_progress = 1;
continue;
@@ -135,6 +140,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
}
usage(fetch_pack_usage);
}
+   if (deepen_not.nr)
+   args.deepen_not = _not;
 
if (i < argc)
dest = argv[i++];
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 283aa95..147504d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -41,6 +41,7 @@ static int max_children = 1;
 static const char *depth;
 static const char *deepen_since;
 static const char *upload_pack;
+static struct string_list deepen_not = STRING_LIST_INIT_NODUP;
 static struct strbuf default_rla = STRBUF_INIT;
 static struct transport *gtransport;
 static struct transport *gsecondary;
@@ -118,6 +119,8 @@ static struct option builtin_fetch_options[] = {
   N_("deepen history of shallow clone")),
OPT_STRING(0, "shallow-since", _since, N_("time"),
   N_("deepen history of shallow repository based on time")),
+   OPT_STRING_LIST(0, "shallow-exclude", _not, N_("revision"),
+   N_("deepen history of shallow clone by excluding rev")),
{ OPTION_SET_INT, 0, "unshallow", , NULL,
   N_("convert to a complete repository"),
   PARSE_OPT_NONEG | PARSE_OPT_NOARG, NULL, 1 },
@@ -875,6 

[PATCH 21/27] upload-pack: support define shallow boundary by excluding revisions

[Nguyễn Thái Ngọc Duy]

This should allow the user to say "create a shallow clone of this branch
after version ".

Short refs are accepted and expanded at the server side with expand_ref()
because we cannot expand (unknown) refs from the client side.

Like deepen-since, deepen-not cannot be used with deepen. But deepen-not
can be mixed with deepen-since. The result is exactly how you do the
command "git rev-list --since=... --not ref".

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 Documentation/technical/pack-protocol.txt |  3 ++-
 Documentation/technical/protocol-capabilities.txt |  9 +
 upload-pack.c | 23 +--
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt 
b/Documentation/technical/pack-protocol.txt
index 9251df1..dee33a6 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -220,7 +220,8 @@ out of what the server said it could do with the first 
'want' line.
   shallow-line  =  PKT-LINE("shallow" SP obj-id)
 
   depth-request =  PKT-LINE("deepen" SP depth) /
-  PKT-LINE("deepen-since" SP timestamp)
+  PKT-LINE("deepen-since" SP timestamp) /
+  PKT-LINE("deepen-not" SP ref)
 
   first-want=  PKT-LINE("want" SP obj-id SP capability-list)
   additional-want   =  PKT-LINE("want" SP obj-id)
diff --git a/Documentation/technical/protocol-capabilities.txt 
b/Documentation/technical/protocol-capabilities.txt
index f08cc4e..0e6b57d 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -188,6 +188,15 @@ specific time, instead of depth. Internally it's 
equivalent of doing
 "rev-list --max-age=" on the server side. "deepen-since"
 cannot be used with "deepen".
 
+deepen-not
+--
+
+This capability adds "deepen-not" command to fetch-pack/upload-pack
+protocol so the client can request shallow clones that are cut at a
+specific revision, instead of depth. Internally it's equivalent of
+doing "rev-list --not " on the server side. "deepen-not"
+cannot be used with "deepen", but can be used with "deepen-since".
+
 no-progress
 ---
 
diff --git a/upload-pack.c b/upload-pack.c
index b0ddfff..3f40fcb 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -643,6 +643,7 @@ static void deepen_by_rev_list(int ac, const char **av,
 static void receive_needs(void)
 {
struct object_array shallows = OBJECT_ARRAY_INIT;
+   struct string_list deepen_not = STRING_LIST_INIT_DUP;
int depth = 0;
int has_non_tip = 0;
unsigned long deepen_since = 0;
@@ -693,6 +694,16 @@ static void receive_needs(void)
deepen_rev_list = 1;
continue;
}
+   if (skip_prefix(line, "deepen-not ", )) {
+   char *ref = NULL;
+   unsigned char sha1[20];
+   if (expand_ref(arg, strlen(arg), sha1, ) != 1)
+   die("git upload-pack: ambiguous deepen-not: 
%s", line);
+   string_list_append(_not, ref);
+   free(ref);
+   deepen_rev_list = 1;
+   continue;
+   }
if (!skip_prefix(line, "want ", ) ||
get_sha1_hex(arg, sha1_buf))
die("git upload-pack: protocol error, "
@@ -747,7 +758,7 @@ static void receive_needs(void)
if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
return;
if (depth > 0 && deepen_rev_list)
-   die("git upload-pack: deepen and deepen-since cannot be used 
together");
+   die("git upload-pack: deepen and deepen-since (or deepen-not) 
cannot be used together");
if (depth > 0)
deepen(depth, );
else if (deepen_rev_list) {
@@ -757,6 +768,14 @@ static void receive_needs(void)
argv_array_push(, "rev-list");
if (deepen_since)
argv_array_pushf(, "--max-age=%lu", deepen_since);
+   if (deepen_not.nr) {
+   argv_array_push(, "--not");
+   for (i = 0; i < deepen_not.nr; i++) {
+   struct string_list_item *s = deepen_not.items + 
i;
+   argv_array_push(, s->string);
+   }
+   argv_array_push(, "--not");
+   }
for (i = 0; i < want_obj.nr; i++) {
struct object *o = want_obj.objects[i].item;
argv_array_push(, oid_to_hex(>oid));
@@ -812,7 +831,7 @@ static int send_ref(const char *refname, const struct 
object_id *oid,
int flag, void *cb_data)
 {
static 

[PATCH 13/27] fetch-pack.c: mark strings for translating

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 fetch-pack.c | 75 ++--
 1 file changed, 37 insertions(+), 38 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 4020744..08caf1d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -208,7 +208,7 @@ static void consume_shallow_list(struct fetch_pack_args 
*args, int fd)
continue;
if (starts_with(line, "unshallow "))
continue;
-   die("git fetch-pack: expected shallow list");
+   die(_("git fetch-pack: expected shallow list"));
}
}
 }
@@ -220,7 +220,7 @@ static enum ack_type get_ack(int fd, unsigned char 
*result_sha1)
const char *arg;
 
if (!len)
-   die("git fetch-pack: expected ACK/NAK, got EOF");
+   die(_("git fetch-pack: expected ACK/NAK, got EOF"));
if (!strcmp(line, "NAK"))
return NAK;
if (skip_prefix(line, "ACK ", )) {
@@ -238,7 +238,7 @@ static enum ack_type get_ack(int fd, unsigned char 
*result_sha1)
return ACK;
}
}
-   die("git fetch_pack: expected ACK/NAK, got '%s'", line);
+   die(_("git fetch_pack: expected ACK/NAK, got '%s'"), line);
 }
 
 static void send_request(struct fetch_pack_args *args,
@@ -285,7 +285,7 @@ static int find_common(struct fetch_pack_args *args,
size_t state_len = 0;
 
if (args->stateless_rpc && multi_ack == 1)
-   die("--stateless-rpc requires multi_ack_detailed");
+   die(_("--stateless-rpc requires multi_ack_detailed"));
if (marked)
for_each_ref(clear_marks, NULL);
marked = 1;
@@ -357,23 +357,23 @@ static int find_common(struct fetch_pack_args *args,
while ((line = packet_read_line(fd[0], NULL))) {
if (skip_prefix(line, "shallow ", )) {
if (get_sha1_hex(arg, sha1))
-   die("invalid shallow line: %s", line);
+   die(_("invalid shallow line: %s"), 
line);
register_shallow(sha1);
continue;
}
if (skip_prefix(line, "unshallow ", )) {
if (get_sha1_hex(arg, sha1))
-   die("invalid unshallow line: %s", line);
+   die(_("invalid unshallow line: %s"), 
line);
if (!lookup_object(sha1))
-   die("object not found: %s", line);
+   die(_("object not found: %s"), line);
/* make sure that it is parsed as shallow */
if (!parse_object(sha1))
-   die("error in object: %s", line);
+   die(_("error in object: %s"), line);
if (unregister_shallow(sha1))
-   die("no shallow found: %s", line);
+   die(_("no shallow found: %s"), line);
continue;
}
-   die("expected shallow/unshallow, got %s", line);
+   die(_("expected shallow/unshallow, got %s"), line);
}
} else if (!args->stateless_rpc)
send_request(args, fd[1], _buf);
@@ -412,8 +412,8 @@ static int find_common(struct fetch_pack_args *args,
do {
ack = get_ack(fd[0], result_sha1);
if (ack)
-   print_verbose(args, "got ack %d %s", 
ack,
- sha1_to_hex(result_sha1));
+   print_verbose(args, _("got %s %d %s"), 
"ack",
+ ack, 
sha1_to_hex(result_sha1));
switch (ack) {
case ACK:
flushes = 0;
@@ -426,7 +426,7 @@ static int find_common(struct fetch_pack_args *args,
struct commit *commit =
lookup_commit(result_sha1);
if (!commit)
-   die("invalid commit %s", 
sha1_to_hex(result_sha1));
+   die(_("invalid commit %s"), 
sha1_to_hex(result_sha1));
if (args->stateless_rpc
   

[PATCH 15/27] shallow.c: implement a generic shallow boundary finder based on rev-list

[Nguyễn Thái Ngọc Duy]

Instead of a custom commit walker like get_shallow_commits(), this new
function uses rev-list to mark NOT_SHALLOW to all reachable commits,
except borders. The definition of reachable is to be defined by the
protocol later. This makes it more flexible to define shallow boundary.

The way we find border is paint all reachable commits NOT_SHALLOW.  Any
of them that "touches" commits without NOT_SHALLOW flag are considered
shallow (e.g. zero parents via grafting mechanism). Shallow commits and
their true parents are all marked SHALLOW. Then NOT_SHALLOW is removed
from shallow commits at the end.

There is an interesting observation. With a generic walker, we can
produce all kinds of shallow cutting. In the following graph, every
commit but "x" is reachable. "b" is a parent of "a".

   x -- a -- o
  //
x -- c -- b -- o

After this function is run, "a" and "c" are both considered shallow
commits. After grafting occurs at the client side, what we see is

a -- o
/
 c -- b -- o

Notice that because of grafting, "a" has zero parents, so "b" is no
longer a parent of "a".

This is unfortunate and may be solved in two ways. The first is change
the way shallow grafting works and keep "a -- b" connection if "b"
exists and always ends at shallow commits (iow, no loose ends). This is
hard to detect, or at least not cheap to do.

The second way is mark one "x" as shallow commit instead of "a" and
produce this graph at client side:

   x -- a -- o
   //
 c -- b -- o

More commits, but simpler grafting rules.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 commit.h  |  2 ++
 shallow.c | 78 +++
 2 files changed, 80 insertions(+)

diff --git a/commit.h b/commit.h
index 5d58be0..b717be1 100644
--- a/commit.h
+++ b/commit.h
@@ -258,6 +258,8 @@ extern int for_each_commit_graft(each_commit_graft_fn, void 
*);
 extern int is_repository_shallow(void);
 extern struct commit_list *get_shallow_commits(struct object_array *heads,
int depth, int shallow_flag, int not_shallow_flag);
+extern struct commit_list *get_shallow_commits_by_rev_list(
+   int ac, const char **av, int shallow_flag, int 
not_shallow_flag);
 extern void set_alternate_shallow_file(const char *path, int override);
 extern int write_shallow_commits(struct strbuf *out, int use_pack_protocol,
 const struct sha1_array *extra);
diff --git a/shallow.c b/shallow.c
index 60f1505..40c2485 100644
--- a/shallow.c
+++ b/shallow.c
@@ -10,6 +10,8 @@
 #include "diff.h"
 #include "revision.h"
 #include "commit-slab.h"
+#include "revision.h"
+#include "list-objects.h"
 
 static int is_shallow = -1;
 static struct stat_validity shallow_stat;
@@ -137,6 +139,82 @@ struct commit_list *get_shallow_commits(struct 
object_array *heads, int depth,
return result;
 }
 
+static void show_commit(struct commit *commit, void *data)
+{
+   commit_list_insert(commit, data);
+}
+
+/*
+ * Given rev-list arguments, run rev-list. All reachable commits
+ * except border ones are marked with not_shallow_flag. Border commits
+ * are marked with shallow_flag. The list of border/shallow commits
+ * are also returned.
+ */
+struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,
+   int shallow_flag,
+   int not_shallow_flag)
+{
+   struct commit_list *result = NULL, *p;
+   struct commit_list *not_shallow_list = NULL;
+   struct rev_info revs;
+   int both_flags = shallow_flag | not_shallow_flag;
+
+   /*
+* SHALLOW (excluded) and NOT_SHALLOW (included) should not be
+* set at this point. But better be safe than sorry.
+*/
+   clear_object_flags(both_flags);
+
+   is_repository_shallow(); /* make sure shallows are read */
+
+   init_revisions(, NULL);
+   save_commit_buffer = 0;
+   setup_revisions(ac, av, , NULL);
+
+   if (prepare_revision_walk())
+   die("revision walk setup failed");
+   traverse_commit_list(, show_commit, NULL, _shallow_list);
+
+   /* Mark all reachable commits as NOT_SHALLOW */
+   for (p = not_shallow_list; p; p = p->next)
+   p->item->object.flags |= not_shallow_flag;
+
+   /*
+* mark border commits SHALLOW + NOT_SHALLOW.
+* We cannot clear NOT_SHALLOW right now. Imagine border
+* commit A is processed first, then commit B, whose parent is
+* A, later. If NOT_SHALLOW on A is cleared at step 1, B
+* itself is considered border at step 2, which is incorrect.
+*/
+   for (p = not_shallow_list; p; p = p->next) {
+   struct commit *c = p->item;
+   struct commit_list *parent;
+
+   if 

[PATCH 16/27] upload-pack: add deepen-since to cut shallow repos based on time

[Nguyễn Thái Ngọc Duy]

This should allow the user to say "create a shallow clone containing the
work from last year" (once the client side is fixed up, of course).

In theory deepen-since and deepen (aka --depth) can be used together to
draw the shallow boundary (whether it's intersection or union is up to
discussion, but if rev-list is used, it's likely intersection). However,
because deepen goes with a custom commit walker, we can't mix the two
yet.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
---
 Documentation/technical/pack-protocol.txt |  3 +-
 Documentation/technical/protocol-capabilities.txt |  9 +
 upload-pack.c | 45 ++-
 3 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/pack-protocol.txt 
b/Documentation/technical/pack-protocol.txt
index c6977bb..9251df1 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -219,7 +219,8 @@ out of what the server said it could do with the first 
'want' line.
 
   shallow-line  =  PKT-LINE("shallow" SP obj-id)
 
-  depth-request =  PKT-LINE("deepen" SP depth)
+  depth-request =  PKT-LINE("deepen" SP depth) /
+  PKT-LINE("deepen-since" SP timestamp)
 
   first-want=  PKT-LINE("want" SP obj-id SP capability-list)
   additional-want   =  PKT-LINE("want" SP obj-id)
diff --git a/Documentation/technical/protocol-capabilities.txt 
b/Documentation/technical/protocol-capabilities.txt
index eaab6b4..f08cc4e 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -179,6 +179,15 @@ This capability adds "deepen", "shallow" and "unshallow" 
commands to
 the  fetch-pack/upload-pack protocol so clients can request shallow
 clones.
 
+deepen-since
+
+
+This capability adds "deepen-since" command to fetch-pack/upload-pack
+protocol so the client can request shallow clones that are cut at a
+specific time, instead of depth. Internally it's equivalent of doing
+"rev-list --max-age=" on the server side. "deepen-since"
+cannot be used with "deepen".
+
 no-progress
 ---
 
diff --git a/upload-pack.c b/upload-pack.c
index b6f3756..b0ddfff 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -14,6 +14,7 @@
 #include "sigchain.h"
 #include "version.h"
 #include "string-list.h"
+#include "argv-array.h"
 
 static const char upload_pack_usage[] = "git upload-pack [--strict] 
[--timeout=] ";
 
@@ -627,11 +628,25 @@ static void deepen(int depth, const struct object_array 
*shallows)
packet_flush(1);
 }
 
+static void deepen_by_rev_list(int ac, const char **av,
+  struct object_array *shallows)
+{
+   struct commit_list *result;
+
+   result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
+   send_shallow(result);
+   free_commit_list(result);
+   send_unshallow(shallows);
+   packet_flush(1);
+}
+
 static void receive_needs(void)
 {
struct object_array shallows = OBJECT_ARRAY_INIT;
int depth = 0;
int has_non_tip = 0;
+   unsigned long deepen_since = 0;
+   int deepen_rev_list = 0;
 
shallow_nr = 0;
for (;;) {
@@ -668,6 +683,16 @@ static void receive_needs(void)
die("Invalid deepen: %s", line);
continue;
}
+   if (skip_prefix(line, "deepen-since ", )) {
+   char *end = NULL;
+   deepen_since = strtoul(arg, , 0);
+   if (!end || *end || !deepen_since ||
+   /* revisions.c's max_age -1 is special */
+   deepen_since == -1)
+   die("Invalid deepen-since: %s", line);
+   deepen_rev_list = 1;
+   continue;
+   }
if (!skip_prefix(line, "want ", ) ||
get_sha1_hex(arg, sha1_buf))
die("git upload-pack: protocol error, "
@@ -719,10 +744,26 @@ static void receive_needs(void)
if (!use_sideband && daemon_mode)
no_progress = 1;
 
-   if (depth == 0 && shallows.nr == 0)
+   if (depth == 0 && !deepen_rev_list && shallows.nr == 0)
return;
+   if (depth > 0 && deepen_rev_list)
+   die("git upload-pack: deepen and deepen-since cannot be used 
together");
if (depth > 0)
deepen(depth, );
+   else if (deepen_rev_list) {
+   struct argv_array av = ARGV_ARRAY_INIT;
+   int i;
+
+   argv_array_push(, "rev-list");
+   if (deepen_since)
+   argv_array_pushf(, "--max-age=%lu", deepen_since);
+   for (i = 0; i < want_obj.nr; i++) {
+   struct object *o =