From: Mark Jamsek <mark@jamsek.com>
Subject: Re: commit executable bit change
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Sat, 19 Oct 2019 21:21:16 +1100

Hi Stefan,

They worked ok for me

> On 19 Oct 2019, at 9:18 pm, Stefan Sperling <stsp@stsp.name> wrote:
> 
> Any comments on these executable bit fixes? Should I just go ahead?
> 
> On Wed, Oct 16, 2019 at 02:09:39PM +0200, Stefan Sperling wrote:
>> On Wed, Oct 16, 2019 at 02:05:16PM +0200, Stefan Sperling wrote:
>>> On Tue, Oct 15, 2019 at 05:06:08PM +0200, Stefan Sperling wrote:
>>>> I have accidentally committed regress/cmdline/integrate.sh without the
>>>> executable bit because of a bug in 'got revert' where the reverted
>>>> file's permissions were overwritten with those of a temporary file.
>>>> 
>>>> This diff fixes that bug, makes xbit changes show up in 'got status', and
>>>> allows 'got commit' to create new commits which contain only xbit changes.
>>>> 
>>>> 'got diff' will currently produce an empty diff for such commits, but that
>>>> is a separate issue. It is possible to create such commits with Git, too.
>>>> 
>>>> No new regression test is added for the 'revert' bug because showing the
>>>> bogus mode change in 'status' already made test_revert_patch_one_change fail.
>>>> 
>>>> Also, Got was only setting the owner's x bit, but not group/other as Git
>>>> would do. This gets fixed here as well.
>>> 
>>> Here is a follow-up diff which applies on top of the previous diff.
>>> 
>>> With this, chmod +x/-x changes are properly handled during 'got update'.
>> 
>> And here is a patch to make 'got diff' and 'tog diff' show file mode changes.
>> 
>> I think the basics are complete with this. There are still parts that
>> could be improved. E.g. unlike 'got status', 'got diff' will not show
>> chmod +x/-x for files in the work tree.
>> And it is impossible to stage file mode changes with 'got stage' but
>> that seems like an acceptable restriction. Implementing support for
>> this would add more code than it is worth, I think.
>> 
>> I am still looking for reviews of all these 3 xbit-related diffs ;-)
>> 
>> 
>> diff f57d86d87012b20b13d9ea2269f28a0afd072dcd ae3c4c0c3c15546e64d83ff0dfe53da4bcc83a63
>> blob - 5423c33764d81bfc3fd2c71dc7f9ca32f22bad4a
>> blob + 15002586c02db1d645ee8138c70c4fa191c0db37
>> --- got/got.c
>> +++ got/got.c
>> @@ -5024,7 +5024,7 @@ static const struct got_error *
>> check_path_prefix_in_diff(void *arg, struct got_blob_object *blob1,
>>     struct got_blob_object *blob2, struct got_object_id *id1,
>>     struct got_object_id *id2, const char *path1, const char *path2,
>> -    struct got_repository *repo)
>> +    mode_t mode1, mode_t mode2, struct got_repository *repo)
>> {
>> 	struct check_path_prefix_arg *a = arg;
>> 
>> blob - 2cbdb362bbc46817c1d3e664247b978b73e7f02c
>> blob + d7f3479403b5d8286740e682f101b99f5f2782b4
>> --- include/got_diff.h
>> +++ include/got_diff.h
>> @@ -44,11 +44,13 @@ const struct got_error *got_diff_blob_file(struct got_
>>  * the second blob contains content on the new side of the diff.
>>  * The set of arguments relating to either blob may be NULL to indicate
>>  * that no content is present on its respective side of the diff.
>> + * File modes from relevant tree objects which contain the blobs may
>> + * also be passed. These will be zero if not available.
>>  */
>> typedef const struct got_error *(*got_diff_blob_cb)(void *,
>>     struct got_blob_object *, struct got_blob_object *,
>>     struct got_object_id *, struct got_object_id *,
>> -    const char *, const char *, struct got_repository *);
>> +    const char *, const char *, mode_t, mode_t, struct got_repository *);
>> 
>> /*
>>  * A pre-defined implementation of got_diff_blob_cb() which appends unidiff
>> @@ -63,7 +65,7 @@ struct got_diff_blob_output_unidiff_arg {
>> const struct got_error *got_diff_blob_output_unidiff(void *,
>>     struct got_blob_object *, struct got_blob_object *,
>>     struct got_object_id *, struct got_object_id *,
>> -    const char *, const char *, struct got_repository *);
>> +    const char *, const char *, mode_t, mode_t, struct got_repository *);
>> 
>> /*
>>  * Compute the differences between two trees and invoke the provided
>> blob - c100c664ee6266b7d278ba0176da2648da0f1053
>> blob + c98502c82d7d8b1ed38dfe6ab2fbf0b1fbb5d095
>> --- lib/diff.c
>> +++ lib/diff.c
>> @@ -38,8 +38,9 @@
>> 
>> static const struct got_error *
>> diff_blobs(struct got_blob_object *blob1, struct got_blob_object *blob2,
>> -    const char *label1, const char *label2, int diff_context,
>> -    int ignore_whitespace, FILE *outfile, struct got_diff_changes *changes)
>> +    const char *label1, const char *label2, mode_t mode1, mode_t mode2,
>> +    int diff_context, int ignore_whitespace, FILE *outfile,
>> +    struct got_diff_changes *changes)
>> {
>> 	struct got_diff_state ds;
>> 	struct got_diff_args args;
>> @@ -110,8 +111,27 @@ diff_blobs(struct got_blob_object *blob1, struct got_b
>> 		flags |= D_IGNOREBLANKS;
>> 
>> 	if (outfile) {
>> -		fprintf(outfile, "blob - %s\n", idstr1);
>> -		fprintf(outfile, "blob + %s\n", idstr2);
>> +		char *modestr1 = NULL, *modestr2 = NULL;
>> +		if (mode1 && mode1 != mode2) {
>> +			if (asprintf(&modestr1, " (mode %o)",
>> +			    mode1 & (S_IRWXU | S_IRWXG | S_IRWXO)) == -1) {
>> +				err = got_error_from_errno("asprintf");
>> +				goto done;
>> +			}
>> +		}
>> +		if (mode2 && mode1 != mode2) {
>> +			if (asprintf(&modestr2, " (mode %o)",
>> +			    mode2 & (S_IRWXU | S_IRWXG | S_IRWXO)) == -1) {
>> +				err = got_error_from_errno("asprintf");
>> +				goto done;
>> +			}
>> +		}
>> +		fprintf(outfile, "blob - %s%s\n", idstr1,
>> +		    modestr1 ? modestr1 : "");
>> +		fprintf(outfile, "blob + %s%s\n", idstr2,
>> +		    modestr2 ? modestr2 : "");
>> +		free(modestr1);
>> +		free(modestr2);
>> 	}
>> 	err = got_diffreg(&res, f1, f2, flags, &args, &ds, outfile, changes);
>> 	got_diff_state_free(&ds);
>> @@ -127,12 +147,12 @@ const struct got_error *
>> got_diff_blob_output_unidiff(void *arg, struct got_blob_object *blob1,
>>     struct got_blob_object *blob2, struct got_object_id *id1,
>>     struct got_object_id *id2, const char *label1, const char *label2,
>> -    struct got_repository *repo)
>> +    mode_t mode1, mode_t mode2, struct got_repository *repo)
>> {
>> 	struct got_diff_blob_output_unidiff_arg *a = arg;
>> 
>> -	return diff_blobs(blob1, blob2, label1, label2, a->diff_context,
>> -	    a->ignore_whitespace, a->outfile, NULL);
>> +	return diff_blobs(blob1, blob2, label1, label2, mode1, mode2,
>> +	    a->diff_context, a->ignore_whitespace, a->outfile, NULL);
>> }
>> 
>> const struct got_error *
>> @@ -140,7 +160,7 @@ got_diff_blob(struct got_blob_object *blob1, struct go
>>     const char *label1, const char *label2, int diff_context,
>>     int ignore_whitespace, FILE *outfile)
>> {
>> -	return diff_blobs(blob1, blob2, label1, label2, diff_context,
>> +	return diff_blobs(blob1, blob2, label1, label2, 0, 0, diff_context,
>> 	    ignore_whitespace, outfile, NULL);
>> }
>> 
>> @@ -255,7 +275,7 @@ got_diff_blob_lines_changed(struct got_diff_changes **
>> 	if (err)
>> 		return err;
>> 
>> -	err = diff_blobs(blob1, blob2, NULL, NULL, 3, 0, NULL, *changes);
>> +	err = diff_blobs(blob1, blob2, NULL, NULL, 0, 0, 3, 0, NULL, *changes);
>> 	if (err) {
>> 		got_diff_free_changes(*changes);
>> 		*changes = NULL;
>> @@ -276,7 +296,7 @@ got_diff_free_changes(struct got_diff_changes *changes
>> }
>> 
>> static const struct got_error *
>> -diff_added_blob(struct got_object_id *id, const char *label,
>> +diff_added_blob(struct got_object_id *id, const char *label, mode_t mode,
>>     struct got_repository *repo, got_diff_blob_cb cb, void *cb_arg)
>> {
>> 	const struct got_error *err;
>> @@ -290,7 +310,7 @@ diff_added_blob(struct got_object_id *id, const char *
>> 	err = got_object_blob_open(&blob, repo, obj, 8192);
>> 	if (err)
>> 		goto done;
>> -	err = cb(cb_arg, NULL, blob, NULL, id, NULL, label, repo);
>> +	err = cb(cb_arg, NULL, blob, NULL, id, NULL, label, 0, mode, repo);
>> done:
>> 	got_object_close(obj);
>> 	if (blob)
>> @@ -300,8 +320,8 @@ done:
>> 
>> static const struct got_error *
>> diff_modified_blob(struct got_object_id *id1, struct got_object_id *id2,
>> -    const char *label1, const char *label2, struct got_repository *repo,
>> -    got_diff_blob_cb cb, void *cb_arg)
>> +    const char *label1, const char *label2, mode_t mode1, mode_t mode2,
>> +    struct got_repository *repo, got_diff_blob_cb cb, void *cb_arg)
>> {
>> 	const struct got_error *err;
>> 	struct got_object *obj1 = NULL;
>> @@ -333,7 +353,8 @@ diff_modified_blob(struct got_object_id *id1, struct g
>> 	if (err)
>> 		goto done;
>> 
>> -	err = cb(cb_arg, blob1, blob2, id1, id2, label1, label2, repo);
>> +	err = cb(cb_arg, blob1, blob2, id1, id2, label1, label2, mode1, mode2,
>> +	    repo);
>> done:
>> 	if (obj1)
>> 		got_object_close(obj1);
>> @@ -347,7 +368,7 @@ done:
>> }
>> 
>> static const struct got_error *
>> -diff_deleted_blob(struct got_object_id *id, const char *label,
>> +diff_deleted_blob(struct got_object_id *id, const char *label, mode_t mode,
>>     struct got_repository *repo, got_diff_blob_cb cb, void *cb_arg)
>> {
>> 	const struct got_error *err;
>> @@ -361,7 +382,7 @@ diff_deleted_blob(struct got_object_id *id, const char
>> 	err = got_object_blob_open(&blob, repo, obj, 8192);
>> 	if (err)
>> 		goto done;
>> -	err = cb(cb_arg, blob, NULL, id, NULL, label, NULL, repo);
>> +	err = cb(cb_arg, blob, NULL, id, NULL, label, NULL, mode, 0, repo);
>> done:
>> 	got_object_close(obj);
>> 	if (blob)
>> @@ -512,11 +533,11 @@ diff_entry_old_new(const struct got_tree_entry *te1,
>> 			    cb, cb_arg, diff_content);
>> 		else {
>> 			if (diff_content)
>> -				err = diff_deleted_blob(te1->id, label1, repo,
>> -				    cb, cb_arg);
>> +				err = diff_deleted_blob(te1->id, label1,
>> +				    te1->mode, repo, cb, cb_arg);
>> 			else
>> 				err = cb(cb_arg, NULL, NULL, te1->id, NULL,
>> -				    label1, NULL, repo);
>> +				    label1, NULL, te1->mode, 0, repo);
>> 		}
>> 		return err;
>> 	} else if (got_object_tree_entry_is_submodule(te2))
>> @@ -528,13 +549,16 @@ diff_entry_old_new(const struct got_tree_entry *te1,
>> 			return diff_modified_tree(te1->id, te2->id,
>> 			    label1, label2, repo, cb, cb_arg, diff_content);
>> 	} else if (S_ISREG(te1->mode) && S_ISREG(te2->mode)) {
>> -		if (!id_match) {
>> +		if (!id_match ||
>> +		    (te1->mode & S_IXUSR) != (te2->mode & S_IXUSR)) {
>> 			if (diff_content)
>> 				return diff_modified_blob(te1->id, te2->id,
>> -				    label1, label2, repo, cb, cb_arg);
>> +				    label1, label2, te1->mode, te2->mode,
>> +				    repo, cb, cb_arg);
>> 			else
>> 				return cb(cb_arg, NULL, NULL, te1->id,
>> -				    te2->id, label1, label2, repo);
>> +				    te2->id, label1, label2, te1->mode,
>> +				    te2->mode, repo);
>> 		}
>> 	}
>> 
>> @@ -562,9 +586,11 @@ diff_entry_new_old(const struct got_tree_entry *te2,
>> 		    diff_content);
>> 
>> 	if (diff_content)
>> -		return diff_added_blob(te2->id, label2, repo, cb, cb_arg);
>> +		return diff_added_blob(te2->id, label2, te2->mode, repo, cb,
>> +		    cb_arg);
>> 
>> -	return cb(cb_arg, NULL, NULL, NULL, te2->id, NULL, label2, repo);
>> +	return cb(cb_arg, NULL, NULL, NULL, te2->id, NULL, label2, 0,
>> +	    te2->mode, repo);
>> }
>> 
>> const struct got_error *
>> blob - edb81fa00de43c1b50eba8f7ba30d5959593e344
>> blob + e289542324a5a24dba52bcccb0d657378875d631
>> --- lib/worktree.c
>> +++ lib/worktree.c
>> @@ -1986,7 +1986,7 @@ static const struct got_error *
>> merge_file_cb(void *arg, struct got_blob_object *blob1,
>>     struct got_blob_object *blob2, struct got_object_id *id1,
>>     struct got_object_id *id2, const char *path1, const char *path2,
>> -    struct got_repository *repo)
>> +    mode_t mode1, mode_t mode2, struct got_repository *repo)
>> {
>> 	static const struct got_error *err = NULL;
>> 	struct merge_file_cb_arg *a = arg;
>> blob - 0454b55efa3ced59507448a4ccee518a3a29f74e
>> blob + 31587c10a62466ca183a27d0985a8816a43d7053
>> --- regress/cmdline/diff.sh
>> +++ regress/cmdline/diff.sh
>> @@ -217,7 +217,8 @@ function test_diff_tag {
>> 	echo "blob - /dev/null" >> $testroot/stdout.expected
>> 	echo -n 'blob + ' >> $testroot/stdout.expected
>> 	got tree -r $testroot/repo -i -c $commit_id2 | grep 'new$' | \
>> -		cut -d' ' -f 1 >> $testroot/stdout.expected
>> +		cut -d' ' -f 1 | tr -d '\n' >> $testroot/stdout.expected
>> +	echo " (mode 644)" >> $testroot/stdout.expected
>> 	echo '--- /dev/null' >> $testroot/stdout.expected
>> 	echo '+++ new' >> $testroot/stdout.expected
>> 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
>> blob - 42ff36721b7a4349961e76289fca5eca64b2a559 (mode 755)
>> blob + a7b1c1d2ee078473b35a60b0ba9068fdac25b9d7 (mode 744)
>> --- regress/cmdline/import.sh
>> +++ regress/cmdline/import.sh
>> @@ -67,25 +67,25 @@ function test_import_basic {
>> 	echo " " >> $testroot/stdout.expected
>> 	echo "diff /dev/null $tree_id" >> $testroot/stdout.expected
>> 	echo "blob - /dev/null" >> $testroot/stdout.expected
>> -	echo "blob + $id_alpha" >> $testroot/stdout.expected
>> +	echo "blob + $id_alpha (mode 644)" >> $testroot/stdout.expected
>> 	echo "--- /dev/null" >> $testroot/stdout.expected
>> 	echo "+++ alpha" >> $testroot/stdout.expected
>> 	echo "@@ -0,0 +1 @@" >> $testroot/stdout.expected
>> 	echo "+alpha" >> $testroot/stdout.expected
>> 	echo "blob - /dev/null" >> $testroot/stdout.expected
>> -	echo "blob + $id_beta" >> $testroot/stdout.expected
>> +	echo "blob + $id_beta (mode 644)" >> $testroot/stdout.expected
>> 	echo "--- /dev/null" >> $testroot/stdout.expected
>> 	echo "+++ beta" >> $testroot/stdout.expected
>> 	echo "@@ -0,0 +1 @@" >> $testroot/stdout.expected
>> 	echo "+beta" >> $testroot/stdout.expected
>> 	echo "blob - /dev/null" >> $testroot/stdout.expected
>> -	echo "blob + $id_zeta" >> $testroot/stdout.expected
>> +	echo "blob + $id_zeta (mode 644)" >> $testroot/stdout.expected
>> 	echo "--- /dev/null" >> $testroot/stdout.expected
>> 	echo "+++ epsilon/zeta" >> $testroot/stdout.expected
>> 	echo "@@ -0,0 +1 @@" >> $testroot/stdout.expected
>> 	echo "+zeta" >> $testroot/stdout.expected
>> 	echo "blob - /dev/null" >> $testroot/stdout.expected
>> -	echo "blob + $id_delta" >> $testroot/stdout.expected
>> +	echo "blob + $id_delta (mode 644)" >> $testroot/stdout.expected
>> 	echo "--- /dev/null" >> $testroot/stdout.expected
>> 	echo "+++ gamma/delta" >> $testroot/stdout.expected
>> 	echo "@@ -0,0 +1 @@" >> $testroot/stdout.expected
>> blob - c847136cca4bae60bd72b654eb15593e7739fc09 (mode 755)
>> blob + ba1b5eaaf04bac104befb9a8defef1522ad1de37 (mode 744)
>> --- regress/cmdline/stage.sh
>> +++ regress/cmdline/stage.sh
>> @@ -1359,8 +1359,9 @@ function test_stage_commit {
>> 	echo '+modified file' >> $testroot/stdout.expected
>> 	echo -n 'blob - ' >> $testroot/stdout.expected
>> 	got tree -r $testroot/repo -i -c $first_commit \
>> -		| grep 'beta$' | cut -d' ' -f 1 \
>> +		| grep 'beta$' | cut -d' ' -f 1 | tr -d '\n' \
>> 		>> $testroot/stdout.expected
>> +	echo " (mode 644)" >> $testroot/stdout.expected
>> 	echo 'blob + /dev/null' >> $testroot/stdout.expected
>> 	echo '--- beta' >> $testroot/stdout.expected
>> 	echo '+++ /dev/null' >> $testroot/stdout.expected
>> @@ -1368,7 +1369,8 @@ function test_stage_commit {
>> 	echo '-beta' >> $testroot/stdout.expected
>> 	echo 'blob - /dev/null' >> $testroot/stdout.expected
>> 	echo -n 'blob + ' >> $testroot/stdout.expected
>> -	cat $testroot/blob_id_foo >> $testroot/stdout.expected
>> +	cat $testroot/blob_id_foo | tr -d '\n' >> $testroot/stdout.expected
>> +	echo " (mode 644)" >> $testroot/stdout.expected
>> 	echo '--- /dev/null' >> $testroot/stdout.expected
>> 	echo '+++ foo' >> $testroot/stdout.expected
>> 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
>> 
>> 
>