Re: [PATCH 1/4] commit: reload cache properly

2013-06-01 Thread Duy Nguyen 
On Thu, May 30, 2013 at 7:58 PM, Thomas Rast tr...@inf.ethz.ch wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 On Thu, May 30, 2013 at 7:17 AM, Thomas Rast tr...@inf.ethz.ch wrote:
 diff --git i/t/t7501-commit.sh w/t/t7501-commit.sh
 index 195e747..1608254 100755
 --- i/t/t7501-commit.sh
 +++ w/t/t7501-commit.sh
 @@ -524,4 +524,16 @@ test_expect_success 'commit a file whose name is a dash' 
 '
 test_i18ngrep  changed, 5 insertions output
  '

 +test_expect_success '--only works on to-be-born branch' '
 +   git checkout --orphan orphan 
 +   echo foo newfile 
 +   git add newfile 
 +   git commit --only newfile -m--only on unborn branch 
 +   cat expected EOF 
 +100644 blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99   newfile
 +EOF
 +   git ls-tree -r HEAD actual 
 +   test_cmp expected actual
 +'
 +
  test_done

Thomas, can you resubmit this as a patch to Junio? It's good that the
test suite covers all correct behaviors (and the incorrect ones).
--
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 1/4] commit: reload cache properly

2013-05-30 Thread Thomas Rast 
Felipe Contreras felipe.contre...@gmail.com writes:

 We are supposedly adding files, to to which cache if 'the_index' is
 discarded?
[...]
   if (!current_head) {
   discard_cache();
 + if (read_cache()  0)
 + die(_(cannot read the index));
   return;
   }

It is not obvious to me that this is a correct change.  discard_cache()
without subsequent reloading could also legitimately be used to empty
the index.  So if you are fixing a bug, please justify the change and
provide a testcase to guard against it in the future.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] commit: reload cache properly

2013-05-30 Thread Felipe Contreras 
On Thu, May 30, 2013 at 7:17 AM, Thomas Rast tr...@inf.ethz.ch wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 We are supposedly adding files, to to which cache if 'the_index' is
 discarded?
 [...]
   if (!current_head) {
   discard_cache();
 + if (read_cache()  0)
 + die(_(cannot read the index));
   return;
   }

 It is not obvious to me that this is a correct change.  discard_cache()
 without subsequent reloading could also legitimately be used to empty
 the index.  So if you are fixing a bug, please justify the change and
 provide a testcase to guard against it in the future.

So istate-initialized is false, yet somebody can still add entries to
the cache? What happens when somebody else tries to initialize this
cache? All the entries there will be lost, even though nobody
discarded it afterwards.

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

Re: [PATCH 1/4] commit: reload cache properly

2013-05-30 Thread Thomas Rast 
Felipe Contreras felipe.contre...@gmail.com writes:

 On Thu, May 30, 2013 at 7:17 AM, Thomas Rast tr...@inf.ethz.ch wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 We are supposedly adding files, to to which cache if 'the_index' is
 discarded?
 [...]
   if (!current_head) {
   discard_cache();
 + if (read_cache()  0)
 + die(_(cannot read the index));
   return;
   }

 It is not obvious to me that this is a correct change.  discard_cache()
 without subsequent reloading could also legitimately be used to empty
 the index.  So if you are fixing a bug, please justify the change and
 provide a testcase to guard against it in the future.

 So istate-initialized is false, yet somebody can still add entries to
 the cache? What happens when somebody else tries to initialize this
 cache? All the entries there will be lost, even though nobody
 discarded it afterwards.

And yet it works, and your patch breaks it.

diff --git i/t/t7501-commit.sh w/t/t7501-commit.sh
index 195e747..1608254 100755
--- i/t/t7501-commit.sh
+++ w/t/t7501-commit.sh
@@ -524,4 +524,16 @@ test_expect_success 'commit a file whose name is a dash' '
test_i18ngrep  changed, 5 insertions output
 '
 
+test_expect_success '--only works on to-be-born branch' '
+   git checkout --orphan orphan 
+   echo foo newfile 
+   git add newfile 
+   git commit --only newfile -m--only on unborn branch 
+   cat expected EOF 
+100644 blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99   newfile
+EOF
+   git ls-tree -r HEAD actual 
+   test_cmp expected actual
+'
+
 test_done


-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] commit: reload cache properly

2013-05-30 Thread Felipe Contreras 
On Thu, May 30, 2013 at 7:58 AM, Thomas Rast tr...@inf.ethz.ch wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 On Thu, May 30, 2013 at 7:17 AM, Thomas Rast tr...@inf.ethz.ch wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 We are supposedly adding files, to to which cache if 'the_index' is
 discarded?
 [...]
   if (!current_head) {
   discard_cache();
 + if (read_cache()  0)
 + die(_(cannot read the index));
   return;
   }

 It is not obvious to me that this is a correct change.  discard_cache()
 without subsequent reloading could also legitimately be used to empty
 the index.  So if you are fixing a bug, please justify the change and
 provide a testcase to guard against it in the future.

 So istate-initialized is false, yet somebody can still add entries to
 the cache? What happens when somebody else tries to initialize this
 cache? All the entries there will be lost, even though nobody
 discarded it afterwards.

 And yet it works, and your patch breaks it.

It might work, but the API doesn't make any sense.

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

Re: [PATCH 1/4] commit: reload cache properly

2013-05-30 Thread Duy Nguyen 
On Thu, May 30, 2013 at 7:17 PM, Thomas Rast tr...@inf.ethz.ch wrote:
 Felipe Contreras felipe.contre...@gmail.com writes:

 We are supposedly adding files, to to which cache if 'the_index' is
 discarded?
 [...]
   if (!current_head) {
   discard_cache();
 + if (read_cache()  0)
 + die(_(cannot read the index));
   return;
   }

 It is not obvious to me that this is a correct change.  discard_cache()
 without subsequent reloading could also legitimately be used to empty
 the index.  So if you are fixing a bug, please justify the change and
 provide a testcase to guard against it in the future.

That discard_cache line I think came from fa9dcf8 (Fix performance
regression for partial commits - 2008-01-13). The code flow back then
was

if (initial_commit)
   discard_cache();

add_remove_files();
/* do something more */

A quick look from current code seems to indicate this pattern is still
valid for creating partial commits, where temporary index will be
thrown away afterwards. But I may be wrong.
--
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