Re: [PATCH 03/19] reset.c: pass pathspec around instead of (prefix, argv) pair

2013-01-11 Thread Duy Nguyen 
On Fri, Jan 11, 2013 at 6:09 AM, Junio C Hamano gits...@pobox.com wrote:
 Or I could hold off nd/parse-pathspec if this series has a better
 chance of graduation first. Decision?

 I am greedy and want to have both ;-)

Apparently I have no problems with your being greedy.

 There is no textual conflict between the two topics at the moment,
 but because the ultimate goal of your series is to remove all uses
 of the pathspec.raw[] field outside the implementation of pathspec
 matching, it might help to rename the field to _private_raw (or
 remove it), and either make get_pathspec() private or disappear, to
 ensure that the compiler will help us catching semantic conflicts
 with new users of it at a late stage of your series.

There are still some uses for get_pathspec() and new call sites won't
cause big problems because they would need init_pathspec() to convert
get_pathspec() results to struct pathspec. I will rename raw[] though.
-- 
Duy
--
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 03/19] reset.c: pass pathspec around instead of (prefix, argv) pair

2013-01-10 Thread Duy Nguyen 
On Thu, Jan 10, 2013 at 2:26 AM, Junio C Hamano gits...@pobox.com wrote:
 Martin von Zweigbergk martinv...@gmail.com writes:

 We use the path arguments in two places in reset.c: in
 interactive_reset() and read_from_tree(). Both of these call
 get_pathspec(), so we pass the (prefix, arv) pair to both
 functions. Move the call to get_pathspec() out of these methods, for
 two reasons: 1) One argument is simpler than two. 2) It lets us use
 the (arguably clearer) if (pathspec) in place of if (i  argc).
 ---
 If I understand correctly, this should be rebased on top of
 nd/parse-pathspec. Please let me know.

 Yeah, this will conflict with the get_pathspec-to-parse_pathspec
 conversion Duy has been working on.

Or I could hold off nd/parse-pathspec if this series has a better
chance of graduation first. Decision?
-- 
Duy
--
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 03/19] reset.c: pass pathspec around instead of (prefix, argv) pair

2013-01-10 Thread Junio C Hamano 
Duy Nguyen pclo...@gmail.com writes:

 On Thu, Jan 10, 2013 at 2:26 AM, Junio C Hamano gits...@pobox.com wrote:
 Martin von Zweigbergk martinv...@gmail.com writes:

 We use the path arguments in two places in reset.c: in
 interactive_reset() and read_from_tree(). Both of these call
 get_pathspec(), so we pass the (prefix, arv) pair to both
 functions. Move the call to get_pathspec() out of these methods, for
 two reasons: 1) One argument is simpler than two. 2) It lets us use
 the (arguably clearer) if (pathspec) in place of if (i  argc).
 ---
 If I understand correctly, this should be rebased on top of
 nd/parse-pathspec. Please let me know.

 Yeah, this will conflict with the get_pathspec-to-parse_pathspec
 conversion Duy has been working on.

 Or I could hold off nd/parse-pathspec if this series has a better
 chance of graduation first. Decision?

I am greedy and want to have both ;-)

Before deciding that, I'd appreciate a second set of eyes giving
Martin's series an independent review, to see if it is going in the
right direction.  I think I didn't spot anything questionable in it
myself, but second opinion always helps.

There is no textual conflict between the two topics at the moment,
but because the ultimate goal of your series is to remove all uses
of the pathspec.raw[] field outside the implementation of pathspec
matching, it might help to rename the field to _private_raw (or
remove it), and either make get_pathspec() private or disappear, to
ensure that the compiler will help us catching semantic conflicts
with new users of it at a late stage of your series.
--
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 03/19] reset.c: pass pathspec around instead of (prefix, argv) pair

2013-01-09 Thread Martin von Zweigbergk 
We use the path arguments in two places in reset.c: in
interactive_reset() and read_from_tree(). Both of these call
get_pathspec(), so we pass the (prefix, arv) pair to both
functions. Move the call to get_pathspec() out of these methods, for
two reasons: 1) One argument is simpler than two. 2) It lets us use
the (arguably clearer) if (pathspec) in place of if (i  argc).
---
If I understand correctly, this should be rebased on top of
nd/parse-pathspec. Please let me know.

 builtin/reset.c | 27 ++-
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 65413d0..045c960 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -153,26 +153,15 @@ static void update_index_from_diff(struct 
diff_queue_struct *q,
}
 }
 
-static int interactive_reset(const char *revision, const char **argv,
-const char *prefix)
-{
-   const char **pathspec = NULL;
-
-   if (*argv)
-   pathspec = get_pathspec(prefix, argv);
-
-   return run_add_interactive(revision, --patch=reset, pathspec);
-}
-
-static int read_from_tree(const char *prefix, const char **argv,
-   unsigned char *tree_sha1, int refresh_flags)
+static int read_from_tree(const char **pathspec, unsigned char *tree_sha1,
+ int refresh_flags)
 {
struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
int index_fd;
struct diff_options opt;
 
memset(opt, 0, sizeof(opt));
-   diff_tree_setup_paths(get_pathspec(prefix, (const char **)argv), opt);
+   diff_tree_setup_paths(pathspec, opt);
opt.output_format = DIFF_FORMAT_CALLBACK;
opt.format_callback = update_index_from_diff;
 
@@ -216,6 +205,7 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
const char *rev = HEAD;
unsigned char sha1[20], *orig = NULL, sha1_orig[20],
*old_orig = NULL, sha1_old_orig[20];
+   const char **pathspec = NULL;
struct commit *commit;
struct strbuf msg = STRBUF_INIT;
const struct option options[] = {
@@ -287,22 +277,25 @@ int cmd_reset(int argc, const char **argv, const char 
*prefix)
die(_(Could not parse object '%s'.), rev);
hashcpy(sha1, commit-object.sha1);
 
+   if (i  argc)
+   pathspec = get_pathspec(prefix, argv + i);
+
if (patch_mode) {
if (reset_type != NONE)
die(_(--patch is incompatible with 
--{hard,mixed,soft}));
-   return interactive_reset(rev, argv + i, prefix);
+   return run_add_interactive(rev, --patch=reset, pathspec);
}
 
/* git reset tree [--] paths... can be used to
 * load chosen paths from the tree into the index without
 * affecting the working tree nor HEAD. */
-   if (i  argc) {
+   if (pathspec) {
if (reset_type == MIXED)
warning(_(--mixed with paths is deprecated; use 'git 
reset -- paths' instead.));
else if (reset_type != NONE)
die(_(Cannot do %s reset with paths.),
_(reset_type_names[reset_type]));
-   return read_from_tree(prefix, argv + i, sha1,
+   return read_from_tree(pathspec, sha1,
quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
}
if (reset_type == NONE)
-- 
1.8.1.rc3.331.g1ef2165

--
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 03/19] reset.c: pass pathspec around instead of (prefix, argv) pair

2013-01-09 Thread Matt Kraai 
On Wed, Jan 09, 2013 at 12:16:00AM -0800, Martin von Zweigbergk wrote:
 We use the path arguments in two places in reset.c: in
 interactive_reset() and read_from_tree(). Both of these call
 get_pathspec(), so we pass the (prefix, arv) pair to both
  ^^^
argv is misspelled.

-- 
Matt
--
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 03/19] reset.c: pass pathspec around instead of (prefix, argv) pair

2013-01-09 Thread Junio C Hamano 
Martin von Zweigbergk martinv...@gmail.com writes:

 We use the path arguments in two places in reset.c: in
 interactive_reset() and read_from_tree(). Both of these call
 get_pathspec(), so we pass the (prefix, arv) pair to both
 functions. Move the call to get_pathspec() out of these methods, for
 two reasons: 1) One argument is simpler than two. 2) It lets us use
 the (arguably clearer) if (pathspec) in place of if (i  argc).
 ---
 If I understand correctly, this should be rebased on top of
 nd/parse-pathspec. Please let me know.

Yeah, this will conflict with the get_pathspec-to-parse_pathspec
conversion Duy has been working on.

Without the interactions with that topic, the conversion seems
straightforward to me, though.


  builtin/reset.c | 27 ++-
  1 file changed, 10 insertions(+), 17 deletions(-)

 diff --git a/builtin/reset.c b/builtin/reset.c
 index 65413d0..045c960 100644
 --- a/builtin/reset.c
 +++ b/builtin/reset.c
 @@ -153,26 +153,15 @@ static void update_index_from_diff(struct 
 diff_queue_struct *q,
   }
  }
  
 -static int interactive_reset(const char *revision, const char **argv,
 -  const char *prefix)
 -{
 - const char **pathspec = NULL;
 -
 - if (*argv)
 - pathspec = get_pathspec(prefix, argv);
 -
 - return run_add_interactive(revision, --patch=reset, pathspec);
 -}
 -
 -static int read_from_tree(const char *prefix, const char **argv,
 - unsigned char *tree_sha1, int refresh_flags)
 +static int read_from_tree(const char **pathspec, unsigned char *tree_sha1,
 +   int refresh_flags)
  {
   struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
   int index_fd;
   struct diff_options opt;
  
   memset(opt, 0, sizeof(opt));
 - diff_tree_setup_paths(get_pathspec(prefix, (const char **)argv), opt);
 + diff_tree_setup_paths(pathspec, opt);
   opt.output_format = DIFF_FORMAT_CALLBACK;
   opt.format_callback = update_index_from_diff;
  
 @@ -216,6 +205,7 @@ int cmd_reset(int argc, const char **argv, const char 
 *prefix)
   const char *rev = HEAD;
   unsigned char sha1[20], *orig = NULL, sha1_orig[20],
   *old_orig = NULL, sha1_old_orig[20];
 + const char **pathspec = NULL;
   struct commit *commit;
   struct strbuf msg = STRBUF_INIT;
   const struct option options[] = {
 @@ -287,22 +277,25 @@ int cmd_reset(int argc, const char **argv, const char 
 *prefix)
   die(_(Could not parse object '%s'.), rev);
   hashcpy(sha1, commit-object.sha1);
  
 + if (i  argc)
 + pathspec = get_pathspec(prefix, argv + i);
 +
   if (patch_mode) {
   if (reset_type != NONE)
   die(_(--patch is incompatible with 
 --{hard,mixed,soft}));
 - return interactive_reset(rev, argv + i, prefix);
 + return run_add_interactive(rev, --patch=reset, pathspec);
   }
  
   /* git reset tree [--] paths... can be used to
* load chosen paths from the tree into the index without
* affecting the working tree nor HEAD. */
 - if (i  argc) {
 + if (pathspec) {
   if (reset_type == MIXED)
   warning(_(--mixed with paths is deprecated; use 'git 
 reset -- paths' instead.));
   else if (reset_type != NONE)
   die(_(Cannot do %s reset with paths.),
   _(reset_type_names[reset_type]));
 - return read_from_tree(prefix, argv + i, sha1,
 + return read_from_tree(pathspec, sha1,
   quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
   }
   if (reset_type == NONE)
--
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